Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1096393005:
Initialize the new WindowProxy when handing off the global object. (Closed)
|
Created:
5 years, 8 months ago by dcheng Modified:
5 years, 7 months ago CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, site-isolation-reviews_chromium.org, vivekg_samsung, vivekg Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionInitialize the new WindowProxy when handing off the global object.
BUG=480676
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194405
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194904
Patch Set 1 #
Total comments: 1
Messages
Total messages: 21 (7 generated)
dcheng@chromium.org changed reviewers: + haraken@chromium.org
I didn't notice this in my testing, because I was always checking object equality with savedReference === window[0], which calls initializeIfNeeded().
https://codereview.chromium.org/1096393005/diff/1/Source/bindings/core/v8/Win... File Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/1096393005/diff/1/Source/bindings/core/v8/Win... Source/bindings/core/v8/WindowProxy.cpp:165: initializeIfNeeded(); Just to confirm: This initializeIfNeeded() creates a new Context and its global object but doesn't set the newly created global object to m_global (because you've already set m_global in the above). Is this expected?
On 2015/04/24 05:06:04, haraken wrote: > https://codereview.chromium.org/1096393005/diff/1/Source/bindings/core/v8/Win... > File Source/bindings/core/v8/WindowProxy.cpp (right): > > https://codereview.chromium.org/1096393005/diff/1/Source/bindings/core/v8/Win... > Source/bindings/core/v8/WindowProxy.cpp:165: initializeIfNeeded(); > > Just to confirm: This initializeIfNeeded() creates a new Context and its global > object but doesn't set the newly created global object to m_global (because > you've already set m_global in the above). Is this expected? I think this is expected for the behavior of takeGlobalFrom, but what concerns me a bit is a mismatch between the m_global and the current context's global object.
On 2015/04/24 at 05:07:00, haraken wrote: > On 2015/04/24 05:06:04, haraken wrote: > > https://codereview.chromium.org/1096393005/diff/1/Source/bindings/core/v8/Win... > > File Source/bindings/core/v8/WindowProxy.cpp (right): > > > > https://codereview.chromium.org/1096393005/diff/1/Source/bindings/core/v8/Win... > > Source/bindings/core/v8/WindowProxy.cpp:165: initializeIfNeeded(); > > > > Just to confirm: This initializeIfNeeded() creates a new Context and its global > > object but doesn't set the newly created global object to m_global (because > > you've already set m_global in the above). Is this expected? > Yes, this is intentional--this is how we keep script references from breaking. Normally, when you navigate, we detach the context--but we don't clear the global object. So when we recreate the context after navigation, it uses the previous global object, so script references still work. For swapping between local and remote, we need to preserve the global object manually, since we create a completely new WindowProxy. > I think this is expected for the behavior of takeGlobalFrom, but what concerns me a bit is a mismatch between the m_global and the current context's global object. For LocalDOMWindows, it's not necessary because we install a new Document on navigation, which calls ScriptController::updateDocument. This leads to WindowProxy::initializeIfNeeded getting called, which re-establishes all the required state. However, RemoteDOMWindow doesn't have anything that triggers the call to initialize the WindowProxy on navigation. Instead, the global object that we previously handed out to script just remains an Object with all the access checks restricted (since we detach the global on navigation). We need to call initializeIfNeeded() to eagerly create the new v8::Context, which associates itself with the global object we previously used. If we don't do this, the v8::Context is only lazily created when someone happens to try to get a remote DOMWindow via a property like window[1]. This leads to very counter-intuitive behavior, since you can have this happen: win = window[0]; win.location = 'https://some-cross-site-page.example.com/'; win.location = 'same-site-page.html'; // This fails with a 'no access' error, since the global object was detached for navigation in the previous line and no context has been re-attached. window[0]; // Asking Blink to vend us another reference for the remote DOMWindow causes the v8::Context for the RemoteDOMWindow to be created. win.location = 'same-site-page.html'; // This now works, since the global object is now reattached to a v8::Context.
On 2015/04/24 06:23:09, dcheng wrote: > On 2015/04/24 at 05:07:00, haraken wrote: > > On 2015/04/24 05:06:04, haraken wrote: > > > > https://codereview.chromium.org/1096393005/diff/1/Source/bindings/core/v8/Win... > > > File Source/bindings/core/v8/WindowProxy.cpp (right): > > > > > > > https://codereview.chromium.org/1096393005/diff/1/Source/bindings/core/v8/Win... > > > Source/bindings/core/v8/WindowProxy.cpp:165: initializeIfNeeded(); > > > > > > Just to confirm: This initializeIfNeeded() creates a new Context and its > global > > > object but doesn't set the newly created global object to m_global (because > > > you've already set m_global in the above). Is this expected? > > > > Yes, this is intentional--this is how we keep script references from breaking. > Normally, when you navigate, we detach the context--but we don't clear the > global object. So when we recreate the context after navigation, it uses the > previous global object, so script references still work. > > For swapping between local and remote, we need to preserve the global object > manually, since we create a completely new WindowProxy. > > > I think this is expected for the behavior of takeGlobalFrom, but what concerns > me a bit is a mismatch between the m_global and the current context's global > object. > > For LocalDOMWindows, it's not necessary because we install a new Document on > navigation, which calls ScriptController::updateDocument. This leads to > WindowProxy::initializeIfNeeded getting called, which re-establishes all the > required state. > > However, RemoteDOMWindow doesn't have anything that triggers the call to > initialize the WindowProxy on navigation. Instead, the global object that we > previously handed out to script just remains an Object with all the access > checks restricted (since we detach the global on navigation). We need to call > initializeIfNeeded() to eagerly create the new v8::Context, which associates > itself with the global object we previously used. If we don't do this, the > v8::Context is only lazily created when someone happens to try to get a remote > DOMWindow via a property like window[1]. This leads to very counter-intuitive > behavior, since you can have this happen: > > win = window[0]; > win.location = 'https://some-cross-site-page.example.com/'; > win.location = 'same-site-page.html'; // This fails with a 'no access' error, > since the global object was detached for navigation in the previous line and no > context has been re-attached. > window[0]; // Asking Blink to vend us another reference for the remote > DOMWindow causes the v8::Context for the RemoteDOMWindow to be created. > win.location = 'same-site-page.html'; // This now works, since the global > object is now reattached to a v8::Context. Thanks for a lot of clarifications. Makes sense. LGTM.
The CQ bit was checked by dcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096393005/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=194405
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1078983007/ by tkent@chromium.org. The reason for reverting is: Speculative revert for RenderFrameImplTest failures blocking Blink roll http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_....
Message was sent while issue was closed.
On 2015/04/26 20:51:00, tkent wrote: > A revert of this CL (patchset #1 id:1) has been created in > https://codereview.chromium.org/1078983007/ by mailto:tkent@chromium.org. > > The reason for reverting is: Speculative revert for RenderFrameImplTest failures > blocking Blink roll > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_.... Confirmed this was the culprit.
dcheng@chromium.org changed reviewers: + yukishiino@chromium.or
+yukishiino So I've been looking into the LSan failures and I can't figure them out. I see that during shutdown, neither LocalDOMWindow nor RemoteDOMWindow's destructor are being called. I'm pretty sure this is because the ScriptWrappable for LocalDOMWindow/RemoteDOMWindow is still holding a reference to it, because v8 has not GCed yet. However, what I can't figure out is why LSan thinks LocalDOMWindow is reachable but the RemoteDOMWindow is not. Do we have to do something special with LocalDOMWindow/v8 for LSan to understand it?
dcheng@chromium.org changed reviewers: + earthdok@chromium.org, yukishiino@chromium.org - yukishiino@chromium.or
+earthdok@, who might be able to give some more insight from the LSan perspective. I still haven't figured out why LSan reports just the RemoteDOMWindow leaking. I poked around some more using a local patch to trace when DOMWindow is reffed and dereffed. I've confirmed that a reference from v8 is keeping both LocalDOMWindow and RemoteDOMWindow alive when the Chrome browser tests shut down. For LocalDOMWindow, this stack looks like: #0 0x0000004e34d1 __interceptor_backtrace #1 0x7f67be087b3e base::debug::StackTrace::StackTrace() #2 0x7f67ab385603 WTF::DebugRefCounted<>::ref() #3 0x7f67ae4c2571 blink::V8Window::refObject() #4 0x7f67ad5e0b88 blink::WrapperTypeInfo::refObject() #5 0x7f67ad5e021a blink::V8DOMWrapper::associateObjectWithWrapper() #6 0x7f67ad8a5aaa blink::WindowProxy::installDOMWindow() #7 0x7f67ad8a333d blink::WindowProxy::initialize() #8 0x7f67ad8a2ca0 blink::WindowProxy::initializeIfNeeded() #9 0x7f67ad6b5eba blink::ScriptController::windowProxy() #10 0x7f67ad6b9c6d blink::ScriptController::namedItemAdded() #11 0x7f67b009926d blink::HTMLDocument::addItemToMap() #12 0x7f67b0099422 blink::HTMLDocument::addExtraNamedItem() #13 0x7f67b00f3d74 blink::HTMLIFrameElement::insertedInto() #14 0x7f67afa5f282 blink::ContainerNode::notifyNodeInsertedInternal() #15 0x7f67afa5b223 blink::ContainerNode::notifyNodeInserted() #16 0x7f67afa587c2 blink::ContainerNode::parserAppendChild() #17 0x7f67b0466a18 blink::insert() #18 0x7f67b045cd09 blink::executeInsertTask() #19 0x7f67b045ca35 blink::HTMLConstructionSite::executeTask() #20 0x7f67b045f6cc blink::HTMLConstructionSite::executeQueuedTasks() #21 0x7f67b0525c7b blink::HTMLTreeBuilder::constructTree() #22 0x7f67b047c5e1 blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken() #23 0x7f67b047bef2 blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser() #24 0x7f67b0478f78 blink::HTMLDocumentParser::pumpPendingSpeculations() #25 0x7f67b047851d blink::HTMLDocumentParser::resumeParsingAfterYield() #26 0x7f67b04bc96d blink::HTMLParserScheduler::continueParsing() #27 0x7f67b04be86b WTF::FunctionWrapper<>::operator()() #28 0x7f67b04be780 WTF::PartBoundFunctionImpl<>::operator()() #29 0x7f67c14d2751 WTF::Function<>::operator()() #30 0x7f67c1b7667e blink::CancellableTaskFactory::CancellableTask::run() #31 0x7f67c62b375b scheduler::WebSchedulerImpl::runTask() #32 0x7f67c62b8031 base::internal::RunnableAdapter<>::Run() #33 0x7f67c62b7cdb _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIPFv10scoped_ptrIN5blink9WebThread4TaskENS_14DefaultDeleterIS6_EEEEEENS0_8TypeListIJS9_EEEE8MakeItSoESC_S9_ #34 0x7f67c62b7b06 _ZN4base8internal7InvokerI13IndexSequenceIJLm0EEENS0_9BindStateINS0_15RunnableAdapterIPFv10scoped_ptrIN5blink9WebThread4TaskENS_14DefaultDeleterIS9_EEEEEESD_NS0_8TypeListIJNS0_13PassedWrapperISC_EEEEEEENSG_IJNS0_12UnwrapTraitsISI_EEEEENS0_12InvokeHelperILb0EvSF_NSG_IJSC_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #35 0x7f67be009cd4 base::Callback<>::Run() #36 0x7f67be0a31e1 base::debug::TaskAnnotator::RunTask() #37 0x7f67c62459ce scheduler::TaskQueueManager::ProcessTaskFromWorkQueue() #38 0x7f67c6241864 scheduler::TaskQueueManager::DoWork() #39 0x7f67c62a71a7 base::internal::RunnableAdapter<>::Run() #40 0x7f67c62a6f26 _ZN4base8internal12InvokeHelperILb1EvNS0_15RunnableAdapterIMN9scheduler16TaskQueueManagerEFvbEEENS0_8TypeListIJRKNS_7WeakPtrIS4_EERKbEEEE8MakeItSoES7_SC_SE_ #41 0x7f67c62a6cfc _ZN4base8internal7InvokerI13IndexSequenceIJLm0ELm1EEENS0_9BindStateINS0_15RunnableAdapterIMN9scheduler16TaskQueueManagerEFvbEEEFvPS7_bENS0_8TypeListIJNS_7WeakPtrIS7_EEbEEEEENSD_IJNS0_12UnwrapTraitsISF_EENSI_IbEEEEENS0_12InvokeHelperILb1EvSA_NSD_IJRKSF_RKbEEEEEFvvEE3RunEPNS0_13BindStateBaseE #42 0x7f67be009cd4 base::Callback<>::Run() #43 0x7f67be0a31e1 base::debug::TaskAnnotator::RunTask() #44 0x7f67be285a8c base::MessageLoop::RunTask() #45 0x7f67be285e8c base::MessageLoop::DeferOrRunPendingTask() #46 0x7f67be2871fe base::MessageLoop::DoWork() #47 0x7f67be2c0448 base::MessagePumpDefault::Run() #48 0x7f67be284bf0 base::MessageLoop::RunHandler() #49 0x7f67be45017c base::RunLoop::Run() #50 0x0000014776a9 content::FrameLoadWaiter::Wait() #51 0x00000147b9ff content::RenderViewTest::LoadHTML() #52 0x000000c4af78 content::RenderFrameImplTest::SetUp() #53 0x00000198f7f4 testing::internal::HandleSehExceptionsInMethodIfSupported<>() #54 0x00000193374d testing::internal::HandleExceptionsInMethodIfSupported<>() #55 0x0000018f7855 testing::Test::Run() #56 0x0000018f936f testing::TestInfo::Run() #57 0x0000018fb515 testing::TestCase::Run() #58 0x00000191596e testing::internal::UnitTestImpl::RunAllTests() #59 0x0000019a3174 testing::internal::HandleSehExceptionsInMethodIfSupported<>() #60 0x00000193b377 testing::internal::HandleExceptionsInMethodIfSupported<>() #61 0x000001914a7d testing::UnitTest::Run() For RemoteDOMWindow, this stack looks like: #0 0x0000004e34d1 __interceptor_backtrace #1 0x7f67be087b3e base::debug::StackTrace::StackTrace() #2 0x7f67ab380287 _ZNSt3__16vectorIN4base5debug10StackTraceENS_9allocatorIS3_EEE24__emplace_back_slow_pathIJEEEvDpOT_ #3 0x7f67ab38564b WTF::DebugRefCounted<>::ref() #4 0x7f67ae4c2571 blink::V8Window::refObject() #5 0x7f67ad5e0b88 blink::WrapperTypeInfo::refObject() #6 0x7f67ad5e021a blink::V8DOMWrapper::associateObjectWithWrapper() #7 0x7f67ad8a5aaa blink::WindowProxy::installDOMWindow() #8 0x7f67ad8a333d blink::WindowProxy::initialize() #9 0x7f67ad8a2ca0 blink::WindowProxy::initializeIfNeeded() #10 0x7f67ad8a2bb8 blink::WindowProxy::takeGlobalFrom() #11 0x7f67ad8aec53 blink::WindowProxyManager::takeGlobalFrom() #12 0x7f67b0eabfee blink::Frame::finishSwapFrom() #13 0x7f67aaa0840e blink::WebFrame::swap() #14 0x7f679f4f4506 content::RenderFrameImpl::OnSwapOut() #15 0x000000c4b13e content::RenderFrameImplTest::SetUp() #16 0x00000198f7f4 testing::internal::HandleSehExceptionsInMethodIfSupported<>() #17 0x00000193374d testing::internal::HandleExceptionsInMethodIfSupported<>() #18 0x0000018f7855 testing::Test::Run() #19 0x0000018f936f testing::TestInfo::Run() #20 0x0000018fb515 testing::TestCase::Run() #21 0x00000191596e testing::internal::UnitTestImpl::RunAllTests() #22 0x0000019a3174 testing::internal::HandleSehExceptionsInMethodIfSupported<>() #23 0x00000193b377 testing::internal::HandleExceptionsInMethodIfSupported<>() #24 0x000001914a7d testing::UnitTest::Run() #25 0x000001671171 RUN_ALL_TESTS() #26 0x00000166eb62 base::TestSuite::Run() #27 0x000000f2dc67 content::ContentTestLauncherDelegate::RunTestSuite() #28 0x0000014b5bec content::LaunchTests() #29 0x000000f2d9c2 main #30 0x7f6795a63ec5 __libc_start_main #31 0x0000004a9864 <unknown> The code that does this is https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... which saves a pointer to the DOMWindow into v8 and refs the object to keep it alive. From https://crbug.com/328552, it looks like LSan can't trace into v8's mmaped pages. So this explains why it considers RemoteDOMWindow a leak. However, I can't figure out why LocalDOMWindow isn't considered a leak--its only reference should be from v8 as well. I guess the other possibility is that there's a raw LocalDOMWindow on the stack or in the registers somewhere... but that seems very unlikely: I think LSan runs just before we exit the process.
The CQ bit was checked by dcheng@chromium.org
The CQ bit was unchecked by dcheng@chromium.org
The CQ bit was checked by dcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096393005/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=194904
Message was sent while issue was closed.
On 2015/05/01 02:17:59, dcheng wrote: > From https://crbug.com/328552, it looks like LSan can't trace into v8's mmaped > pages. Actually, it can. The problem is with tests that shut down V8 before LSan starts leak checking. Everything that inherits from RenderViewTest (such as in this case) is guilty of this. > So this explains why it considers RemoteDOMWindow a leak. However, I > can't figure out why LocalDOMWindow isn't considered a leak--its only reference > should be from v8 as well. I guess the other possibility is that there's a raw > LocalDOMWindow on the stack or in the registers somewhere... but that seems very > unlikely: I think LSan runs just before we exit the process. You can try LSAN_OPTIONS=log_pointers=true. This will tell you where LSan found the pointer to the LocalDOMWindow. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
