Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1161983005:
FrameLoader: Prevent reentrancy on dispatchDidClearWindowObjectInMainWorld. (Closed)
DescriptionFrameLoader: Prevent reentrancy on dispatchDidClearWindowObjectInMainWorld.
Sometimes, the event triggered by dispatchDidClearWindowObjectInMainWorld can
trigger yet another dispatch of this event. This leads to unexpected reentrant
behavior. See https://codereview.chromium.org/1148223004/ comment #6.
Also this has bitten someone else in the past. See
https://code.google.com/p/chromium/issues/detail?id=422244.
BUG=493889
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196182
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : fix broken patchset #Patch Set 5 : fix broken patchset #Messages
Total messages: 22 (8 generated)
tommycli@chromium.org changed reviewers: + chrishtr@chromium.org
chrishtr: PTAL. Thanks for the suggestion.
https://codereview.chromium.org/1161983005/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1161983005/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.cpp:1375: if (m_dispatchingDidClearWindowObjectInMainWorld) I think you should do this instead: if (m_dispatchingDidClearWindowObjectInMainWorld) return m_dispatchingDidClearWindowObjectInMainWorld = true; ... dispatch m_dispatchingDidClearWindowObjectInMainWorld = false; https://codereview.chromium.org/1161983005/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.h (right): https://codereview.chromium.org/1161983005/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.h:259: bool m_dispatchingDidClearWindowObjectInMainWorld; Initialize to false in the constructor.
On 2015/05/28 22:06:07, tommycli wrote: > chrishtr: PTAL. Thanks for the suggestion. Do we know how or why this is happening? It seems like this might be papering over a bug somewhere else.
https://codereview.chromium.org/1161983005/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1161983005/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.cpp:1375: if (m_dispatchingDidClearWindowObjectInMainWorld) On 2015/05/28 22:09:25, chrishtr wrote: > I think you should do this instead: > > if (m_dispatchingDidClearWindowObjectInMainWorld) > return > m_dispatchingDidClearWindowObjectInMainWorld = true; > > ... dispatch > > m_dispatchingDidClearWindowObjectInMainWorld = false; Done. https://codereview.chromium.org/1161983005/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.h (right): https://codereview.chromium.org/1161983005/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.h:259: bool m_dispatchingDidClearWindowObjectInMainWorld; On 2015/05/28 22:09:25, chrishtr wrote: > Initialize to false in the constructor. Done.
On 2015/05/28 22:11:01, Nate Chapin wrote: > On 2015/05/28 22:06:07, tommycli wrote: > > chrishtr: PTAL. Thanks for the suggestion. > > Do we know how or why this is happening? It seems like this might be papering > over a bug somewhere else. Hi Nate, the below stacktrace shows how this is happening. The handler for didClearWindowObject calls WebLocalFrame::mainWorldScriptContext() and that triggers another dispatch of the didClearWindowObject. I'm not sure why exactly. Here's an example stacktrace: #0 0x7fc9d51fd52e base::debug::StackTrace::StackTrace() #1 0x7fc9e3726583 plugins::PluginPlaceholder::BindWebFrame() #2 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame() #3 0x7fc9e372864e WebViewPlugin::didClearWindowObject() #4 0x7fc9e372869c WebViewPlugin::didClearWindowObject() #5 0x7fc9d6701359 blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld() #6 0x7fc9c93893c8 blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld() #7 0x7fc9c851d346 blink::ScriptController::windowProxy() #8 0x7fc9c91dae9f blink::LocalFrame::windowProxy() #9 0x7fc9c855f76a blink::toV8Context() #10 0x7fc9d67b20fa blink::WebLocalFrameImpl::mainWorldScriptContext() #11 0x7fc9e37265c1 plugins::PluginPlaceholder::BindWebFrame() #12 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame() #13 0x7fc9e372864e WebViewPlugin::didClearWindowObject() #14 0x7fc9e372869c WebViewPlugin::didClearWindowObject() #15 0x7fc9d6701359 blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld() #16 0x7fc9c93840a9 blink::FrameLoader::dispatchDidClearDocumentOfWindowObject() #17 0x7fc9c9383eb6 blink::FrameLoader::receivedFirstData() #18 0x7fc9c936bf1a blink::DocumentLoader::ensureWriter() #19 0x7fc9c936a49e blink::DocumentLoader::commitData()
Nate, do you have feedback on possible root causes or other issues with the CL? I'm not that familiar with the DidClearWindowObjectInMainWorld event dispatch code.
chrishtr@chromium.org changed reviewers: + japhet@chromium.org
On 2015/05/29 21:11:21, chrishtr wrote: > Nate, do you have feedback on possible root causes or other issues with the CL? > I'm not that familiar with the DidClearWindowObjectInMainWorld event dispatch > code. Ok, that makes sense. I'm assuming this is plugin-specific? If so, I'd prefer to prevent the re-entrancy in plugin-specific code (maybe WebViewPlugin::didClearWindowObject?)
On 2015/05/29 21:16:10, Nate Chapin wrote: > On 2015/05/29 21:11:21, chrishtr wrote: > > Nate, do you have feedback on possible root causes or other issues with the > CL? > > I'm not that familiar with the DidClearWindowObjectInMainWorld event dispatch > > code. > > Ok, that makes sense. I'm assuming this is plugin-specific? If so, I'd prefer to > prevent the re-entrancy in plugin-specific code (maybe > WebViewPlugin::didClearWindowObject?) Hi Nate, It's not purely plugin specific, as it has bitten someone else also. See https://code.google.com/p/chromium/issues/detail?id=422244
Ok. I tend to get grouchy when state gets added to FrameLoader, but I guess there isn't a good alternative. LGTM with a nit. https://codereview.chromium.org/1161983005/diff/20001/Source/core/loader/Fram... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1161983005/diff/20001/Source/core/loader/Fram... Source/core/loader/FrameLoader.cpp:1381: m_dispatchingDidClearWindowObjectInMainWorld = true; TemporaryChange<bool> inDidClearWindowObject(m_dispatchingDidClearWindowObjectInMainWorld, true);
Nate: Thanks. I guess the more ideal fix would be to somehow make ScriptController::windowProxy no longer trigger a second dispatch. This band-aid should probably still go in, but that seems like the 'real' fix. Not sure who owns that code. https://codereview.chromium.org/1161983005/diff/20001/Source/core/loader/Fram... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1161983005/diff/20001/Source/core/loader/Fram... Source/core/loader/FrameLoader.cpp:1381: m_dispatchingDidClearWindowObjectInMainWorld = true; On 2015/05/29 21:28:00, Nate Chapin wrote: > TemporaryChange<bool> > inDidClearWindowObject(m_dispatchingDidClearWindowObjectInMainWorld, true); Done.
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1161983005/#ps40001 (title: " ")
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1161983005/#ps80001 (title: "fix broken patchset")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161983005/80001
The CQ bit was unchecked by tommycli@chromium.org
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161983005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196182 |
