WindowProxy::setGlobal() can be called on an already-initialized WindowProxy |
|||||
Issue descriptionFrom crash reports with debugging CHECKs, the stack looks like this: 0x000007fed4b09024 (chrome_child.dll -windowproxy.cpp:160 ) blink::WindowProxy::initializeIfNeeded() 0x000007fed46cb865 (chrome_child.dll -windowproxymanager.cpp:71 ) blink::WindowProxyManagerBase::setGlobals(WTF::HashMap<blink::DOMWrapperWorld *,v8::Local<v8::Object>,WTF::PtrHash<blink::DOMWrapperWorld>,WTF::HashTraits<blink::DOMWrapperWorld *>,WTF::HashTraits<v8::Local<v8::Object> >,WTF::PartitionAllocator> const &) 0x000007fed4706aa6 (chrome_child.dll -webframe.cpp:111 ) blink::WebFrame::swap(blink::WebFrame *) 0x000007fed5d93536 (chrome_child.dll -render_frame_impl.cc:5048 ) content::RenderFrameImpl::SwapIn() 0x000007fed4b04724 (chrome_child.dll -render_frame_impl.cc:3613 ) content::RenderFrameImpl::didCommitProvisionalLoad(blink::WebLocalFrame *,blink::WebHistoryItem const &,blink::WebHistoryCommitType) This crash is on the CHECK at https://chromium.googlesource.com/chromium/src/+/7cf0339964c7dcb8b77660a31bed2467b9f7cc3a/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp#160: CHECK(V8Window::hasInstance(globalProxy, m_isolate)); The strange part is this crash is in the already initialized branch of initializeIfNeeded(), but this crash is during frame swap, when no script should have yet run in the frame. From https://codereview.chromium.org/2737553007/#msg8, this is a problem because: - We call setGlobal() to transfer the global proxy during frame swap - setGlobal() unconditionally overwrites m_globalProxy - But context->Global() is already initialized and points to another global proxy - In teardown, we clear the wrapper class ID on m_globalProxy, but clear the internal fields on context->Global(). Now, context->Global() has null internal fields but still has a wrapper class ID. It turns out this is because context initialization order is currently non-deterministic when swapping frames. From https://codereview.chromium.org/2743653002/: Repro steps: 1. Install several extensions; 2. Visit a number of pages in a tab; 3. Do a lot of back and forward navigations quickly (not waiting till pages load). This is caused by setting the global object in WindowProxy after it was initialized. This happens when WindowProxyManagerBase::setGlobals processes a non-main world first. It sends out a didCreateScriptContext call. ExtensionFrameHelper::DidCreateScriptContext wants to know whether it is the main world script context, and compares with WebLocalFrameImpl::mainWorldScriptContext(). As a result, main world script context is initialized. At this point the call stack is like this: 0 libbase.dylib 0x0000000111917c1c base::debug::StackTrace::StackTrace(unsigned long) + 28 1 libblink_core.dylib 0x0000000119a376af blink::LocalWindowProxy::initialize() + 527 2 libblink_core.dylib 0x0000000119a3cfe9 blink::ScriptController::windowProxy(blink::DOMWrapperWorld&) + 25 3 libblink_core.dylib 0x0000000119a571bf blink::toV8Context(blink::Frame*, blink::DOMWrapperWorld&) + 15 4 libblink_core.dylib 0x0000000119a46345 blink::ScriptState::forMainWorld(blink::LocalFrame*) + 53 5 libblink_web.dylib 0x000000011915d78d non-virtual thunk to blink::WebLocalFrameImpl::mainWorldScriptContext() const + 13 6 libchrome_dll.dylib 0x000000010be8f1a1 extensions::ExtensionFrameHelper::DidCreateScriptContext(v8::Local<v8::Context>, int) + 49 7 libcontent.dylib 0x000000011416a5b7 content::RenderFrameImpl::didCreateScriptContext(blink::WebLocalFrame*, v8::Local<v8::Context>, int) + 215 8 libblink_core.dylib 0x0000000119a3790d blink::LocalWindowProxy::initialize() + 1133 9 libblink_core.dylib 0x0000000119a73aa8 blink::WindowProxy::setGlobal(v8::Local<v8::Object>) + 152 10 libblink_core.dylib 0x0000000119a74646 blink::WindowProxyManagerBase::setGlobals(WTF::HashMap<blink::DOMWrapperWorld*, v8::Local<v8::Object>, WTF::PtrHash<blink::DOMWrapperWorld>, WTF::HashTraits<blink::DOMWrapperWorld*>, WTF::HashTraits<v8::Local<v8::Object> >, WTF::PartitionAllocator> const&) + 262 11 libblink_web.dylib 0x000000011914b174 blink::WebFrame::swap(blink::WebFrame*) + 612 12 libcontent.dylib 0x000000011415afcb content::RenderFrameImpl::SwapIn() + 235 13 libcontent.dylib 0x00000001141634b6 content::RenderFrameImpl::didCommitProvisionalLoad(blink::WebLocalFrame*, blink::WebHistoryItem const&, blink::WebHistoryCommitType) + 534 After that WindowProxyManagerBase::setGlobals continues its work and sets the global object for the main world WindowProxy, and it becomes broken.
,
Mar 9 2017
Separately, we should be fixing ExtensionFrameHelper to not call back into Blink here. I'll handle that in a separate CL, and after that, let's try sprinkling in some CHECKs into the initialization/detach paths.
,
Mar 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1993f6e8c0fe533f411ae5764231072e212221ad commit 1993f6e8c0fe533f411ae5764231072e212221ad Author: ahest <ahest@yandex-team.ru> Date: Fri Mar 10 01:33:55 2017 Make context initialization order deterministic in WebFrame::swap(). DCHECK(m_globalProxy != context()->Global()) sometimes fail. In builds with DCHECKs turned off this issue manifests with "Type Error: no access" errors in console and scripts not working. Started happening approx. in M56. Repro steps: 1. Install several extensions; 2. Visit a number of pages in a tab; 3. Do a lot of back and forward navigations quickly (not waiting till pages load). This is caused by setting the global object in WindowProxy after it was initialized. This happens when WindowProxyManagerBase::setGlobals processes a non-main world first. It sends out a didCreateScriptContext call. ExtensionFrameHelper::DidCreateScriptContext wants to know whether it is the main world script context, and compares with WebLocalFrameImpl::mainWorldScriptContext(). As a result, main world script context is initialized. At this point the call stack is like this: 1 blink::LocalWindowProxy::initialize 2 blink::ScriptController::windowProxy 3 blink::toV8Context 4 blink::ScriptState::forMainWorld 5 blink::WebLocalFrameImpl::mainWorldScriptContext 6 extensions::ExtensionFrameHelper::DidCreateScriptContext 7 content::RenderFrameImpl::didCreateScriptContext 8 blink::LocalWindowProxy::initialize 9 blink::WindowProxy::setGlobal 10 blink::WindowProxyManagerBase::setGlobals 11 blink::WebFrame::swap 12 content::RenderFrameImpl::SwapIn 13 content::RenderFrameImpl::didCommitProvisionalLoad 14 blink::FrameLoaderClientImpl::dispatchDidCommitLoad 15 blink::FrameLoader::receivedFirstData 16 blink::DocumentLoader::createWriterFor 17 blink::DocumentLoader::ensureWriter 18 blink::DocumentLoader::commitData After that WindowProxyManagerBase::setGlobals continues its work and sets the global object for the main world WindowProxy, and it becomes broken. BUG= 700077 Review-Url: https://codereview.chromium.org/2743653002 Cr-Commit-Position: refs/heads/master@{#455951} [modify] https://crrev.com/1993f6e8c0fe533f411ae5764231072e212221ad/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp [modify] https://crrev.com/1993f6e8c0fe533f411ae5764231072e212221ad/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h [modify] https://crrev.com/1993f6e8c0fe533f411ae5764231072e212221ad/third_party/WebKit/Source/web/WebFrame.cpp
,
Mar 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/128ef2d7248bd291e08ddfa30c8444dc26abc9b8 commit 128ef2d7248bd291e08ddfa30c8444dc26abc9b8 Author: dcheng <dcheng@chromium.org> Date: Fri Mar 10 03:09:29 2017 ExtensionFrameHelper shouldn't trigger initialization of the main world DidCreateScriptContext() compares the context argument to the main world script context. However, getting the main world script context will initialize its WindowProxy, which can corrupt internal state when swapping frames. BUG= 700077 Review-Url: https://codereview.chromium.org/2740873005 Cr-Commit-Position: refs/heads/master@{#455975} [modify] https://crrev.com/128ef2d7248bd291e08ddfa30c8444dc26abc9b8/extensions/renderer/extension_frame_helper.cc
,
Mar 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8b77cc6ed024fb219d6de46378bf18eb05ae7f6 commit e8b77cc6ed024fb219d6de46378bf18eb05ae7f6 Author: dcheng <dcheng@chromium.org> Date: Fri Mar 10 07:24:46 2017 Add CHECKs in WindowProxy initialization/detach to enforce invariants This changes the DCHECKs in disposeContext() to CHECKs and adds a new CHECK in setGlobal(). This should help catch situations where: - an already initialized global proxy object is overwritten but not reinitialized: this can happen during WebFrame::swap() if the embedder tries to access other script contexts, leading to out of sync state. - the global proxy object that WindowProxy retains and the global proxy object retained by the v8::Context differ, also leading to out of sync state. BUG= 700077 Review-Url: https://codereview.chromium.org/2741923002 Cr-Commit-Position: refs/heads/master@{#456016} [modify] https://crrev.com/e8b77cc6ed024fb219d6de46378bf18eb05ae7f6/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp [modify] https://crrev.com/e8b77cc6ed024fb219d6de46378bf18eb05ae7f6/third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp [modify] https://crrev.com/e8b77cc6ed024fb219d6de46378bf18eb05ae7f6/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
,
Mar 10 2017
#1 No idea why it would trigger a lot more than before. Maybe different code generation reorded this slightly so that it reliable triggered. In any case it's awesome that this gets sorted out!
,
Mar 11 2017
There may a repro for this in issue 700510 (using the extension attached to issue 700346 ). See comment 4 for the DCHECKs I'm failing (which dcheng@ just upgraded to CHECKs in r456016).
,
Mar 21 2017
,
Mar 21 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/679313e28ae3a1dfab27fa6c0920fad29d1163b5 commit 679313e28ae3a1dfab27fa6c0920fad29d1163b5 Author: Daniel Cheng <dcheng@chromium.org> Date: Tue Mar 21 18:50:07 2017 Make context initialization order deterministic in WebFrame::swap(). DCHECK(m_globalProxy != context()->Global()) sometimes fail. In builds with DCHECKs turned off this issue manifests with "Type Error: no access" errors in console and scripts not working. Started happening approx. in M56. Repro steps: 1. Install several extensions; 2. Visit a number of pages in a tab; 3. Do a lot of back and forward navigations quickly (not waiting till pages load). This is caused by setting the global object in WindowProxy after it was initialized. This happens when WindowProxyManagerBase::setGlobals processes a non-main world first. It sends out a didCreateScriptContext call. ExtensionFrameHelper::DidCreateScriptContext wants to know whether it is the main world script context, and compares with WebLocalFrameImpl::mainWorldScriptContext(). As a result, main world script context is initialized. At this point the call stack is like this: 1 blink::LocalWindowProxy::initialize 2 blink::ScriptController::windowProxy 3 blink::toV8Context 4 blink::ScriptState::forMainWorld 5 blink::WebLocalFrameImpl::mainWorldScriptContext 6 extensions::ExtensionFrameHelper::DidCreateScriptContext 7 content::RenderFrameImpl::didCreateScriptContext 8 blink::LocalWindowProxy::initialize 9 blink::WindowProxy::setGlobal 10 blink::WindowProxyManagerBase::setGlobals 11 blink::WebFrame::swap 12 content::RenderFrameImpl::SwapIn 13 content::RenderFrameImpl::didCommitProvisionalLoad 14 blink::FrameLoaderClientImpl::dispatchDidCommitLoad 15 blink::FrameLoader::receivedFirstData 16 blink::DocumentLoader::createWriterFor 17 blink::DocumentLoader::ensureWriter 18 blink::DocumentLoader::commitData After that WindowProxyManagerBase::setGlobals continues its work and sets the global object for the main world WindowProxy, and it becomes broken. BUG= 700077 Review-Url: https://codereview.chromium.org/2743653002 Cr-Commit-Position: refs/heads/master@{#455951} (cherry picked from commit 1993f6e8c0fe533f411ae5764231072e212221ad) Review-Url: https://codereview.chromium.org/2764883003 . Cr-Commit-Position: refs/branch-heads/3029@{#337} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/679313e28ae3a1dfab27fa6c0920fad29d1163b5/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp [modify] https://crrev.com/679313e28ae3a1dfab27fa6c0920fad29d1163b5/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h [modify] https://crrev.com/679313e28ae3a1dfab27fa6c0920fad29d1163b5/third_party/WebKit/Source/web/WebFrame.cpp
,
Mar 24 2017
,
Mar 24 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by dcheng@chromium.org
, Mar 9 2017