Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19670020:
Run the Port cleanup code inside BindToGC in a setTimeout call to avoid (Closed)
|
Created:
7 years, 5 months ago by not at google - send to devlin Modified:
7 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRun the JS callbacks that execute on a v8 GC via MiscellaneousBindings::BoundToGC
in the current MessageLoop rather than re-entrantly, to avoid executing any JS in an
unexpected state.
BUG=258526
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213372
Patch Set 1 #Patch Set 2 : comment tweak #
Total comments: 2
Patch Set 3 : . #Patch Set 4 : use message loop #
Total comments: 11
Patch Set 5 : . #Patch Set 6 : dont need trycatch anymore #Patch Set 7 : more #
Total comments: 4
Patch Set 8 : more #
Total comments: 4
Patch Set 9 : creation empty #Patch Set 10 : comment #Messages
Total messages: 23 (0 generated)
I'll use setTimeout here after all. Despite it feeling slightly unrobost, there's no good way to do it reliably from C++.
It looks like miscellaneous_bindings.cc:GCCallback could pretty easily yield to the RenderThread instead of calling args->callback synchronously? https://codereview.chromium.org/19670020/diff/3/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right): https://codereview.chromium.org/19670020/diff/3/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/miscellaneous_bindings.js:181: // while executing code which reasonably assumes it hasn't been. "it hasn't been" what?
My worry with deferring to C++ in any way is that we'd be entering back into a v8 state that we don't know anything about; I'd rather let v8 handle the intricacies of that; however given we hold onto the GC-callback as a Persistent handle, I actually can't see much difference either way. Adam, the code Jeffrey is referring to is here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... is it safe to just to a MessageLoop::current()->Post binding that GCCallbackArgs and basically executing that method that way? Or is there some v8 trickiness needed? https://codereview.chromium.org/19670020/diff/3/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right): https://codereview.chromium.org/19670020/diff/3/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/miscellaneous_bindings.js:181: // while executing code which reasonably assumes it hasn't been. On 2013/07/23 20:48:36, Jeffrey Yasskin wrote: > "it hasn't been" what? Done.
lgtm assuming Adam finds a reason we shouldn't do this in C++.
Oh, could you comment this aspect of BindToGC's behavior in its header file?
On 2013/07/23 21:03:40, kalman wrote: > My worry with deferring to C++ in any way is that we'd be entering back into a > v8 state that we don't know anything about; I'd rather let v8 handle the > intricacies of that; however given we hold onto the GC-callback as a Persistent > handle, I actually can't see much difference either way. > > Adam, the code Jeffrey is referring to is here: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... > > is it safe to just to a MessageLoop::current()->Post binding that GCCallbackArgs > and basically executing that method that way? Or is there some v8 trickiness > needed? That would make me feel much better about this code, actually. Running JS in the middle of GC seems like it might in itself be undefined behavior. Posting in C++ seems like the right way to go, presuming you don't still need |object| to be alive, only |callback|. > https://codereview.chromium.org/19670020/diff/3/chrome/renderer/resources/ext... > File chrome/renderer/resources/extensions/miscellaneous_bindings.js (right): > > https://codereview.chromium.org/19670020/diff/3/chrome/renderer/resources/ext... > chrome/renderer/resources/extensions/miscellaneous_bindings.js:181: // while > executing code which reasonably assumes it hasn't been. > On 2013/07/23 20:48:36, Jeffrey Yasskin wrote: > > "it hasn't been" what? > > Done.
On Tue, Jul 23, 2013 at 2:59 PM, <adamk@chromium.org> wrote: > On 2013/07/23 21:03:40, kalman wrote: >> >> My worry with deferring to C++ in any way is that we'd be entering back >> into a >> v8 state that we don't know anything about; I'd rather let v8 handle the >> intricacies of that; however given we hold onto the GC-callback as a > > Persistent >> >> handle, I actually can't see much difference either way. > > >> Adam, the code Jeffrey is referring to is here: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex... > >> is it safe to just to a MessageLoop::current()->Post binding that > > GCCallbackArgs >> >> and basically executing that method that way? Or is there some v8 >> trickiness >> needed? > > > That would make me feel much better about this code, actually. Running JS in > the > middle of GC seems like it might in itself be undefined behavior. Posting in > C++ > seems like the right way to go, presuming you don't still need |object| to > be > alive, only |callback|. https://code.google.com/p/chromium/codesearch/#chromium/src/v8/src/global-han... does say that "arbitrary API functions." are safe in these callbacks, but I agree that it seems safer to do this in C++.
Pretty sure this is sane. I rewrote it a bit to make sure that message loop shutdown doesn't cause leakage of v8 state... assuming that DeleteSoon does guarantee that.
(and comment added. ptal)
https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:180: GCCallback* self) { I think you want to call self->object_.reset() here to allow |object| to die.
https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:180: GCCallback* self) { On 2013/07/23 22:47:38, adamk wrote: > I think you want to call > > self->object_.reset() > > here to allow |object| to die. ah good point.
I don't think there's any point in the TryCatch anymore either.
https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:180: GCCallback* self) { On 2013/07/23 22:47:38, adamk wrote: > I think you want to call > > self->object_.reset() > > here to allow |object| to die. And possibly link to https://code.google.com/p/chromium/codesearch/#chromium/src/v8/include/v8.h&l... since the obvious place, https://code.google.com/p/chromium/codesearch/#chromium/src/v8/include/v8.h&l... doesn't say anything about needing to do that. https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:183: base::MessageLoop::current()->DeleteSoon(FROM_HERE, self); I'd just PostTask(Bind(&GCCallback::DoStuff, Owned(self))). DeleteSoon doesn't do anything special to actually delete objects if the MessageLoop has shut down, but Bind(Owned()) would delete the object immediately. https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:183: base::MessageLoop::current()->DeleteSoon(FROM_HERE, self); After posting to the MessageLoop, how do we know that callback_ still has a valid reference to V8? https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:210: new GCCallback(args[0].As<v8::Object>(), args[1].As<v8::Function>()); If possible, I'd rather not have code that looks like an obvious leak. Make it a static method or call object_.MakeWeak() out here so readers can see where ownership of the pointer probably went.
done or something. not so happy about the public-ness of the destructor but what you gonna do, OwnedWrapper is in base::internal and internal stuff is ... internal, right. https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:180: GCCallback* self) { On 2013/07/23 22:59:35, Jeffrey Yasskin wrote: > On 2013/07/23 22:47:38, adamk wrote: > > I think you want to call > > > > self->object_.reset() > > > > here to allow |object| to die. > > And possibly link to > https://code.google.com/p/chromium/codesearch/#chromium/src/v8/include/v8.h&l... > since the obvious place, > https://code.google.com/p/chromium/codesearch/#chromium/src/v8/include/v8.h&l... > doesn't say anything about needing to do that. Added comment (though not to the source itself, arbitrary line numbers will change...) https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:183: base::MessageLoop::current()->DeleteSoon(FROM_HERE, self); On 2013/07/23 22:59:35, Jeffrey Yasskin wrote: > I'd just PostTask(Bind(&GCCallback::DoStuff, Owned(self))). DeleteSoon doesn't > do anything special to actually delete objects if the MessageLoop has shut down, > but Bind(Owned()) would delete the object immediately. Done. https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:183: base::MessageLoop::current()->DeleteSoon(FROM_HERE, self); On 2013/07/23 22:59:35, Jeffrey Yasskin wrote: > After posting to the MessageLoop, how do we know that callback_ still has a > valid reference to V8? callback_ is Persistent, so it will "by definition". Though Adam, do we need to do something special when webkit decides to shut down v8?
https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:183: base::MessageLoop::current()->DeleteSoon(FROM_HERE, self); On 2013/07/23 23:17:41, kalman wrote: > On 2013/07/23 22:59:35, Jeffrey Yasskin wrote: > > After posting to the MessageLoop, how do we know that callback_ still has a > > valid reference to V8? > > callback_ is Persistent, so it will "by definition". > > Though Adam, do we need to do something special when webkit decides to shut down > v8? All the entry points check for a dead Isolate before trying to do anything, so I don't believe there's anything you need to do here... https://codereview.chromium.org/19670020/diff/27001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:188: v8::Handle<v8::Context> context = callback_->CreationContext(); ...except that this Handle will be empty if the isolate's dead (e.g., due to OOM), so the next line would crash. I guess you'd have to check for context.IsEmpty() to be totally safe, but crashing is likely fine in that case (in fact, the OOM should have already caused a crash). Sad that the C++ version of this turns out to be more complicated, but I still think it's better than forcing the callers to know about this stuff. As long as this code is only ever used on the main thread, I think the code is likely fine as is.
https://codereview.chromium.org/19670020/diff/46001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/46001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:171: new GCCallback(object, callback); Now this looks like an obvious leak. Just move the object_.MakeWeak into this function. https://codereview.chromium.org/19670020/diff/46001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:202: // Just something to bind to so that this can be deleted via base::Owned. Do the work in here, not in the destructor. Destructors should generally not have visible side-effects, and in this case, if the MessageLoop is gone, you almost certainly want to just delete the GCCallback, and not call back into Javascript in the middle of the gc.
https://codereview.chromium.org/19670020/diff/46001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/46001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:171: new GCCallback(object, callback); On 2013/07/23 23:52:47, Jeffrey Yasskin wrote: > Now this looks like an obvious leak. Just move the object_.MakeWeak into this > function. Not obvious to me, but ok done. https://codereview.chromium.org/19670020/diff/46001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:202: // Just something to bind to so that this can be deleted via base::Owned. On 2013/07/23 23:52:47, Jeffrey Yasskin wrote: > Do the work in here, not in the destructor. Destructors should generally not > have visible side-effects, and in this case, if the MessageLoop is gone, you > almost certainly want to just delete the GCCallback, and not call back into > Javascript in the middle of the gc. Done.
lgtm with one comment https://codereview.chromium.org/19670020/diff/51001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/51001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:194: v8::Handle<v8::Context> context = callback_->CreationContext(); I would recommend checking for non-emptyness on the context and just bailing out if it's empty.
lgtm, thanks. https://codereview.chromium.org/19670020/diff/51001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/51001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:182: // Delete this to execute |callback| in destructor to be safe across This comment is out of date. Probably you don't need it anymore.
done and done https://codereview.chromium.org/19670020/diff/51001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/51001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:194: v8::Handle<v8::Context> context = callback_->CreationContext(); On 2013/07/24 00:18:05, adamk wrote: > I would recommend checking for non-emptyness on the context and just bailing out > if it's empty. CreationContext of an object can be empty?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kalman@chromium.org/19670020/54001
https://codereview.chromium.org/19670020/diff/51001/chrome/renderer/extension... File chrome/renderer/extensions/miscellaneous_bindings.cc (right): https://codereview.chromium.org/19670020/diff/51001/chrome/renderer/extension... chrome/renderer/extensions/miscellaneous_bindings.cc:194: v8::Handle<v8::Context> context = callback_->CreationContext(); On 2013/07/24 00:21:09, kalman wrote: > On 2013/07/24 00:18:05, adamk wrote: > > I would recommend checking for non-emptyness on the context and just bailing > out > > if it's empty. > > CreationContext of an object can be empty? It can return an empty handle if the V8 heap is in a bad state. See the ON_BAILOUT macro in v8's api.cc. But in non-dead-isolate cases, no, the CreationContext cannot be empty (and in fact the function will hold a strong reference to it, keeping alive the entire Global object until we release our eference.
Message was sent while issue was closed.
Change committed as 213372 |
