Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23688004:
Change the PepperInProcessRouter to defer resource destruction messages. (Closed)
|
Created:
7 years, 3 months ago by bbudge Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChange the PepperInProcessRouter to defer resource destruction messages.
This changes the in process "proxy" so it posts tasks to send resource destruction messages
instead of calling them directly. This prevents several kinds of reentrancy into the plugin-side
code. In this case, when a URLLoader is released, the plugin can finish before the host cancels
the load and potentially deletes the instance.
BUG=276368
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222614
Patch Set 1 #
Total comments: 1
Patch Set 2 : More detailed comment, only change in-process behavior. #
Total comments: 1
Patch Set 3 : Change in-process router to post tasks for async messages to host. #
Total comments: 1
Patch Set 4 : Attempt to fix in-process router #Patch Set 5 : Restrict change to resource destruction messages. #Patch Set 6 : Follow previous behavior more closely. #
Total comments: 1
Patch Set 7 : Comment explaining fix. #
Messages
Total messages: 21 (0 generated)
Yuzhu for OWNERS review. Tom since he's done similar fixes in the past. Abhishek FYI
LGTM with nits. https://codereview.chromium.org/23688004/diff/1/ppapi/proxy/url_loader_resour... File ppapi/proxy/url_loader_resource.cc (right): https://codereview.chromium.org/23688004/diff/1/ppapi/proxy/url_loader_resour... ppapi/proxy/url_loader_resource.cc:71: // Destroying a URLLoaderResource can cause the page to tear down the plugin I don't quite understand under what circumstances destroying URLLoaderResource will cause the page to tear down the plugin. Would you please talk about that?
I made the comment more detailed and only change in-process behavior. Is the code to detect in-process too cryptic?
LGTM with nit. https://codereview.chromium.org/23688004/diff/16001/ppapi/proxy/url_loader_re... File ppapi/proxy/url_loader_resource.cc (right): https://codereview.chromium.org/23688004/diff/16001/ppapi/proxy/url_loader_re... ppapi/proxy/url_loader_resource.cc:78: if (PpapiGlobals::Get()->GetProxyLock() == NULL) nit: (!PpapiGlobals::Get()->GetProxyLock())
Sorry for the change in direction. The previous approach helped but it was still difficult to prevent cases where the instance could be deleted in the middle of an instance method. Since this is a bigger change in behavior, if this lands on trunk we should wait a little before merging back.
On 2013/09/10 00:05:14, bbudge1 wrote: > Sorry for the change in direction. The previous approach helped but it was still > difficult to prevent cases where the instance could be deleted in the middle of > an instance method. > > Since this is a bigger change in behavior, if this lands on trunk we should wait > a little before merging back. LGTM Thanks!
Sadly, this fails browser_tests for Graphics2D, FileIO, and FileRef. It appears to be when the callback type is force-async. +David for any ideas on how to fix this or work around it.
If you look at all the test failures, they are the first call using the Resource after it was created, and failing before it has a chance to worry about waiting on a response. I think somehow the PP_Resource that is set up in the "Create" call is getting removed from the ResourceTracker before the call that tries to use it. I'm guessing it has something to do with that message to do the Create still sitting in the MessageLoop queue, but I have to read more code to figure out how resource creation works in-process. https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/p... File content/renderer/pepper/pepper_in_process_router.cc (right): https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/p... content/renderer/pepper/pepper_in_process_router.cc:114: base::MessageLoop::current()->PostTask( Wow, we should have been doing this from the very beginning with PepperInProcessRouter. Thanks for fixing it.
On 2013/09/10 18:34:34, dmichael wrote: > If you look at all the test failures, they are the first call using the Resource > after it was created, and failing before it has a chance to worry about waiting > on a response. I think somehow the PP_Resource that is set up in the "Create" > call is getting removed from the ResourceTracker before the call that tries to > use it. I'm guessing it has something to do with that message to do the Create > still sitting in the MessageLoop queue, but I have to read more code to figure > out how resource creation works in-process. > > https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/p... > File content/renderer/pepper/pepper_in_process_router.cc (right): > > https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/p... > content/renderer/pepper/pepper_in_process_router.cc:114: > base::MessageLoop::current()->PostTask( > Wow, we should have been doing this from the very beginning with > PepperInProcessRouter. Thanks for fixing it. I believe it was this way earlier. Look at the annotated subversion view in code search.
Hi, I guess the failure is because in PepperInProcessRouter::SendToHost() sync messages are handled directly while async messages are handled by posted tasks. It is possible that a sync message arrives at the host side while the create message (async) is still waiting in the message loop. :/ On Tue, Sep 10, 2013 at 12:59 PM, <bbudge@chromium.org> wrote: > On 2013/09/10 18:34:34, dmichael wrote: > >> If you look at all the test failures, they are the first call using the >> > Resource > >> after it was created, and failing before it has a chance to worry about >> > waiting > >> on a response. I think somehow the PP_Resource that is set up in the >> "Create" >> call is getting removed from the ResourceTracker before the call that >> tries to >> use it. I'm guessing it has something to do with that message to do the >> Create >> still sitting in the MessageLoop queue, but I have to read more code to >> figure >> out how resource creation works in-process. >> > > > https://codereview.chromium.**org/23688004/diff/25001/** > content/renderer/pepper/**pepper_in_process_router.cc<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc> > >> File content/renderer/pepper/**pepper_in_process_router.cc (right): >> > > > https://codereview.chromium.**org/23688004/diff/25001/** > content/renderer/pepper/**pepper_in_process_router.cc#**newcode114<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc#newcode114> > >> content/renderer/pepper/**pepper_in_process_router.cc:**114: >> base::MessageLoop::current()->**PostTask( >> Wow, we should have been doing this from the very beginning with >> PepperInProcessRouter. Thanks for fixing it. >> > > I believe it was this way earlier. Look at the annotated subversion view > in code > search. > > https://codereview.chromium.**org/23688004/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Sep 10, 2013 at 2:05 PM, Yuzhu Shen <yzshen@chromium.org> wrote: > Hi, > > I guess the failure is because in PepperInProcessRouter::SendToHost() sync > messages are handled directly while async messages are handled by posted > tasks. It is possible that a sync message arrives at the host side while > the create message (async) is still waiting in the message loop. :/ > That was my first thought, but once I looked at the failures, I didn't see a place where a sync message would have happened. E.g., the FileRef test fails here: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_f... GetName is called immediately after creating the FileRef, and returns a var that was set by the FileRefResource constructor. So long as the resource is found, I think this shouldn't fail. I don't see a way a sync message would have interfered with it working. > > > On Tue, Sep 10, 2013 at 12:59 PM, <bbudge@chromium.org> wrote: > >> On 2013/09/10 18:34:34, dmichael wrote: >> >>> If you look at all the test failures, they are the first call using the >>> >> Resource >> >>> after it was created, and failing before it has a chance to worry about >>> >> waiting >> >>> on a response. I think somehow the PP_Resource that is set up in the >>> "Create" >>> call is getting removed from the ResourceTracker before the call that >>> tries to >>> use it. I'm guessing it has something to do with that message to do the >>> Create >>> still sitting in the MessageLoop queue, but I have to read more code to >>> figure >>> out how resource creation works in-process. >>> >> >> >> https://codereview.chromium.**org/23688004/diff/25001/** >> content/renderer/pepper/**pepper_in_process_router.cc<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc> >> >>> File content/renderer/pepper/**pepper_in_process_router.cc (right): >>> >> >> >> https://codereview.chromium.**org/23688004/diff/25001/** >> content/renderer/pepper/**pepper_in_process_router.cc#**newcode114<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc#newcode114> >> >>> content/renderer/pepper/**pepper_in_process_router.cc:**114: >>> base::MessageLoop::current()->**PostTask( >>> Wow, we should have been doing this from the very beginning with >>> PepperInProcessRouter. Thanks for fixing it. >>> >> >> I believe it was this way earlier. Look at the annotated subversion view >> in code >> search. >> >> https://codereview.chromium.**org/23688004/<https://codereview.chromium.org/2... >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Sep 10, 2013 at 1:13 PM, David Michael <dmichael@chromium.org>wrote: > On Tue, Sep 10, 2013 at 2:05 PM, Yuzhu Shen <yzshen@chromium.org> wrote: > >> Hi, >> >> I guess the failure is because in PepperInProcessRouter::SendToHost() >> sync messages are handled directly while async messages are handled by >> posted tasks. It is possible that a sync message arrives at the host side >> while the create message (async) is still waiting in the message loop. :/ >> > That was my first thought, but once I looked at the failures, I didn't see > a place where a sync message would have happened. > > E.g., the FileRef test fails here: > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_f... > GetName is called immediately after creating the FileRef, and returns a > var that was set by the FileRefResource constructor. So long as the > resource is found, I think this shouldn't fail. I don't see a way a sync > message would have interfered with it working. > I am sorry I cannot debug right now (in a meeting), here is what I think is happening: FileSystem is using the new host/resource design, so create is async. However FileRef is still using the old design, so its create is sync. FileRef creation fails because at the moment, it cannot find the FileSystem whose create message is still waiting in the message loop. > > > >> >> >> On Tue, Sep 10, 2013 at 12:59 PM, <bbudge@chromium.org> wrote: >> >>> On 2013/09/10 18:34:34, dmichael wrote: >>> >>>> If you look at all the test failures, they are the first call using the >>>> >>> Resource >>> >>>> after it was created, and failing before it has a chance to worry about >>>> >>> waiting >>> >>>> on a response. I think somehow the PP_Resource that is set up in the >>>> "Create" >>>> call is getting removed from the ResourceTracker before the call that >>>> tries to >>>> use it. I'm guessing it has something to do with that message to do the >>>> Create >>>> still sitting in the MessageLoop queue, but I have to read more code to >>>> figure >>>> out how resource creation works in-process. >>>> >>> >>> >>> https://codereview.chromium.**org/23688004/diff/25001/** >>> content/renderer/pepper/**pepper_in_process_router.cc<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc> >>> >>>> File content/renderer/pepper/**pepper_in_process_router.cc (right): >>>> >>> >>> >>> https://codereview.chromium.**org/23688004/diff/25001/** >>> content/renderer/pepper/**pepper_in_process_router.cc#**newcode114<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc#newcode114> >>> >>>> content/renderer/pepper/**pepper_in_process_router.cc:**114: >>>> base::MessageLoop::current()->**PostTask( >>>> Wow, we should have been doing this from the very beginning with >>>> PepperInProcessRouter. Thanks for fixing it. >>>> >>> >>> I believe it was this way earlier. Look at the annotated subversion view >>> in code >>> search. >>> >>> https://codereview.chromium.**org/23688004/<https://codereview.chromium.org/2... >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Sep 10, 2013 at 2:25 PM, Yuzhu Shen <yzshen@chromium.org> wrote: > > > > On Tue, Sep 10, 2013 at 1:13 PM, David Michael <dmichael@chromium.org>wrote: > >> On Tue, Sep 10, 2013 at 2:05 PM, Yuzhu Shen <yzshen@chromium.org> wrote: >> >>> Hi, >>> >>> I guess the failure is because in PepperInProcessRouter::SendToHost() >>> sync messages are handled directly while async messages are handled by >>> posted tasks. It is possible that a sync message arrives at the host side >>> while the create message (async) is still waiting in the message loop. :/ >>> >> That was my first thought, but once I looked at the failures, I didn't >> see a place where a sync message would have happened. >> >> E.g., the FileRef test fails here: >> >> https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_f... >> GetName is called immediately after creating the FileRef, and returns a >> var that was set by the FileRefResource constructor. So long as the >> resource is found, I think this shouldn't fail. I don't see a way a sync >> message would have interfered with it working. >> > > I am sorry I cannot debug right now (in a meeting), here is what I think > is happening: > FileSystem is using the new host/resource design, so create is async. > However FileRef is still using the old design, so its create is sync. > FileRef creation fails because at the moment, it cannot find the > FileSystem whose create message is still waiting in the message loop. > Yep, you're right. I was confused, because locally I still had Justin's FileRef refactor change. I also was thinking about the PluginResource, which *does* already exist, and is in the tracker. The trouble is that the Host doesn't exist yet. Now that I'm up-to-date, I'm able to reproduce it. PPB_FileRef_Impl::CreateInternal fails here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... I'm playing with the idea of making the PepperInProcessRouter look for the async Graphics2D and FileSystem creation messages, and run only those particular ones immediately. > > >> >> >> >>> >>> >>> On Tue, Sep 10, 2013 at 12:59 PM, <bbudge@chromium.org> wrote: >>> >>>> On 2013/09/10 18:34:34, dmichael wrote: >>>> >>>>> If you look at all the test failures, they are the first call using the >>>>> >>>> Resource >>>> >>>>> after it was created, and failing before it has a chance to worry about >>>>> >>>> waiting >>>> >>>>> on a response. I think somehow the PP_Resource that is set up in the >>>>> "Create" >>>>> call is getting removed from the ResourceTracker before the call that >>>>> tries to >>>>> use it. I'm guessing it has something to do with that message to do >>>>> the Create >>>>> still sitting in the MessageLoop queue, but I have to read more code >>>>> to figure >>>>> out how resource creation works in-process. >>>>> >>>> >>>> >>>> https://codereview.chromium.**org/23688004/diff/25001/** >>>> content/renderer/pepper/**pepper_in_process_router.cc<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc> >>>> >>>>> File content/renderer/pepper/**pepper_in_process_router.cc (right): >>>>> >>>> >>>> >>>> https://codereview.chromium.**org/23688004/diff/25001/** >>>> content/renderer/pepper/**pepper_in_process_router.cc#**newcode114<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc#newcode114> >>>> >>>>> content/renderer/pepper/**pepper_in_process_router.cc:**114: >>>>> base::MessageLoop::current()->**PostTask( >>>>> Wow, we should have been doing this from the very beginning with >>>>> PepperInProcessRouter. Thanks for fixing it. >>>>> >>>> >>>> I believe it was this way earlier. Look at the annotated subversion >>>> view in code >>>> search. >>>> >>>> https://codereview.chromium.**org/23688004/<https://codereview.chromium.org/2... >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Sep 10, 2013 at 2:25 PM, Yuzhu Shen <yzshen@chromium.org> wrote: > > > > On Tue, Sep 10, 2013 at 1:13 PM, David Michael <dmichael@chromium.org>wrote: > >> On Tue, Sep 10, 2013 at 2:05 PM, Yuzhu Shen <yzshen@chromium.org> wrote: >> >>> Hi, >>> >>> I guess the failure is because in PepperInProcessRouter::SendToHost() >>> sync messages are handled directly while async messages are handled by >>> posted tasks. It is possible that a sync message arrives at the host side >>> while the create message (async) is still waiting in the message loop. :/ >>> >> That was my first thought, but once I looked at the failures, I didn't >> see a place where a sync message would have happened. >> >> E.g., the FileRef test fails here: >> >> https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_f... >> GetName is called immediately after creating the FileRef, and returns a >> var that was set by the FileRefResource constructor. So long as the >> resource is found, I think this shouldn't fail. I don't see a way a sync >> message would have interfered with it working. >> > > I am sorry I cannot debug right now (in a meeting), here is what I think > is happening: > FileSystem is using the new host/resource design, so create is async. > However FileRef is still using the old design, so its create is sync. > FileRef creation fails because at the moment, it cannot find the > FileSystem whose create message is still waiting in the message loop. > Yep, you're right. I was confused, because locally I still had Justin's FileRef refactor change. I also was thinking about the PluginResource, which *does* already exist, and is in the tracker. The trouble is that the Host doesn't exist yet. Now that I'm up-to-date, I'm able to reproduce it. PPB_FileRef_Impl::CreateInternal fails here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... I'm playing with the idea of making the PepperInProcessRouter look for the async Graphics2D and FileSystem creation messages, and run only those particular ones immediately. > > >> >> >> >>> >>> >>> On Tue, Sep 10, 2013 at 12:59 PM, <bbudge@chromium.org> wrote: >>> >>>> On 2013/09/10 18:34:34, dmichael wrote: >>>> >>>>> If you look at all the test failures, they are the first call using the >>>>> >>>> Resource >>>> >>>>> after it was created, and failing before it has a chance to worry about >>>>> >>>> waiting >>>> >>>>> on a response. I think somehow the PP_Resource that is set up in the >>>>> "Create" >>>>> call is getting removed from the ResourceTracker before the call that >>>>> tries to >>>>> use it. I'm guessing it has something to do with that message to do >>>>> the Create >>>>> still sitting in the MessageLoop queue, but I have to read more code >>>>> to figure >>>>> out how resource creation works in-process. >>>>> >>>> >>>> >>>> https://codereview.chromium.**org/23688004/diff/25001/** >>>> content/renderer/pepper/**pepper_in_process_router.cc<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc> >>>> >>>>> File content/renderer/pepper/**pepper_in_process_router.cc (right): >>>>> >>>> >>>> >>>> https://codereview.chromium.**org/23688004/diff/25001/** >>>> content/renderer/pepper/**pepper_in_process_router.cc#**newcode114<https://codereview.chromium.org/23688004/diff/25001/content/renderer/pepper/pepper_in_process_router.cc#newcode114> >>>> >>>>> content/renderer/pepper/**pepper_in_process_router.cc:**114: >>>>> base::MessageLoop::current()->**PostTask( >>>>> Wow, we should have been doing this from the very beginning with >>>>> PepperInProcessRouter. Thanks for fixing it. >>>>> >>>> >>>> I believe it was this way earlier. Look at the annotated subversion >>>> view in code >>>> search. >>>> >>>> https://codereview.chromium.**org/23688004/<https://codereview.chromium.org/2... >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Umm, weird. I was playing around locally and used "git cl patch" to get this and debug it. Well, when I uploaded, it uploaded it here. This seems to fix FileRef locally, though Graphics2D might still have problems. And... I'm not certain this won't introduce new problems.
Thanks, David! It is hard to tell whether it introduces new problems. But I don't have better idea. It seems the most natural way to ensure that in-process mode has the same message ordering as out-of-process mode is to use a separate thread as the main thread for in-process plugins, so that it can post tasks for sync messages to the main renderer thread and block. But I doubt that we would like to invest in such a change, considering that in-process mode is going away. On Tue, Sep 10, 2013 at 2:38 PM, <dmichael@chromium.org> wrote: > Umm, weird. I was playing around locally and used "git cl patch" to get > this and > debug it. Well, when I uploaded, it uploaded it here. This seems to fix > FileRef > locally, though Graphics2D might still have problems. And... I'm not > certain > this won't introduce new problems. > > > https://codereview.chromium.**org/23688004/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've restricted the change to resource destruction messages. This should be all I need to fix the bug. All browser tests pass.
I think this is a good approach. The Destroy message should be the last for a given resource, so making it async doesn't seem like it can hurt. Meaning... it might be delayed behind other messages (including sync ones), but it should be the last that refers to a given resource anyway. So I like it. lgtm https://codereview.chromium.org/23688004/diff/54001/content/renderer/pepper/p... File content/renderer/pepper/pepper_in_process_router.cc (right): https://codereview.chromium.org/23688004/diff/54001/content/renderer/pepper/p... content/renderer/pepper/pepper_in_process_router.cc:113: if (message->type() == PpapiHostMsg_ResourceDestroyed::ID) { I think this deserves a beefier comment. Maybe reference the bug, too.
On 2013/09/11 15:44:17, dmichael wrote: > I think this is a good approach. The Destroy message should be the last for a > given resource, so making it async doesn't seem like it can hurt. Meaning... it > might be delayed behind other messages (including sync ones), but it should be > the last that refers to a given resource anyway. So I like it. > > lgtm > > https://codereview.chromium.org/23688004/diff/54001/content/renderer/pepper/p... > File content/renderer/pepper/pepper_in_process_router.cc (right): > > https://codereview.chromium.org/23688004/diff/54001/content/renderer/pepper/p... > content/renderer/pepper/pepper_in_process_router.cc:113: if (message->type() == > PpapiHostMsg_ResourceDestroyed::ID) { > I think this deserves a beefier comment. Maybe reference the bug, too. I like it too. Thanks! LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/23688004/64001
Message was sent while issue was closed.
Change committed as 222614 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
