fast/loader/open-in-srcdoc-unload.html leaks a SuspendableObject |
||||||||||||||||||||
Issue descriptionThe fast/loader/open-in-srcdoc-unload.html test seems to be leaking a Document object. Adding some instrumentation we can see that 4 Documents are created but only 3 are destroyed: ERR: Document 0x31060fe81830\n ERR: Document 0x31060fe82518\n ERR: Document 0x31060fe83318\n ERR: Document 0x31060fe84000\n ERR: ~Document 0x31060fe81830\n ERR: ~Document 0x31060fe83318\n ERR: ~Document 0x31060fe84000\n This is making the test fail once Document starts owning an IntersectionObserverController (https://codereview.chromium.org/2272773002/), which is an ActiveDOMObject: fast/loader/open-in-srcdoc-unload.html failed unexpectedly (leak detected: ({"numberOfLiveActiveDOMObjects":[2,3]}))
,
Nov 1 2016
,
Nov 1 2016
,
Nov 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3c364c812da0312a104e976ca5838507efe3488e commit 3c364c812da0312a104e976ca5838507efe3488e Author: skyostil <skyostil@chromium.org> Date: Tue Nov 01 17:56:08 2016 Use intersection observer to control frame throttling This patch removes the FrameView-specific code for computing viewport visibility in favor of registering an internal intersection observer. BUG= 616519 , 647393 ,661182 Review-Url: https://codereview.chromium.org/2272773002 Cr-Commit-Position: refs/heads/master@{#429046} [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/LayoutTests/LeakExpectations [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/LayoutTests/http/tests/activedomobject/media-expected.txt [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/LayoutTests/http/tests/activedomobject/media.html [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/dom/Document.cpp [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/dom/ElementVisibilityObserver.h [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/dom/IntersectionObserver.cpp [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/dom/IntersectionObserver.h [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/frame/FrameView.cpp [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/frame/FrameView.h [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/layout/svg/LayoutSVGModelObject.cpp [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.cpp [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/core/layout/svg/SVGLayoutSupport.h [modify] https://crrev.com/3c364c812da0312a104e976ca5838507efe3488e/third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp
,
Nov 2 2016
Removing the untriaged status as this is being worked on. Also adding skyostil@ as owner.
,
Nov 2 2016
Sorry, I'm currently not actively working on this. I only wanted to file the bug to make sure this issue is known and maybe get some hints about where the problem might be (or even if this is an actual problem)
,
Nov 5 2016
haraken@ got ideas about the leak?
,
Nov 5 2016
Failures on the leak detector needs to be treated as a tree closer (although we haven't yet made it a real tree closer due to the capacity of our infras). c.f., https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/LeakExpectations?q=leakexpectations&sq=package:chromium&dr&l=5 If it's not easy to identify & fix the leak, we might want to consider reverting the CL.
,
Nov 7 2016
Correct me if I'm wrong, but I think the leak has been there all along -- my patch just made it trip the leak checker. In other words I don't think reverting is going to solve anything.
,
Nov 7 2016
I'm just behind but hasn't the leak started after landing https://codereview.chromium.org/2272773002/ ? Also #0 is saying that the leak started because the CL added a strong reference from Document to IntersectionObserverController, but the CL is not adding the strong reference.
,
Nov 7 2016
Right, the strong reference was there before, but it was only initialized on demand (i.e., when someone registered an intervention observer for the document). One of the side effects of that patch is the registration of an intersection observer. Thinking about this more, reverting the patch would mean we don't leak an IntersectionObserverController but we still leak the document object. Any idea why the document leak isn't caught by the test?
,
Nov 7 2016
Document leaks should be caught by the leak detector... Do you mean that the document leaks in your manual environment but doesn't leak in the leak detector?
,
Nov 7 2016
What I mean is when I run the test locally, I see 4 documents being created and only 3 destroyed, and my IntersectionObserver patch doesn't change these numbers. It increases the number of ActiveDOMObjects which makes the leak detector unhappy.
,
Nov 8 2016
hajimehoshi@: Do you have any idea on #13?
,
Nov 8 2016
Maybe one of the 4 documents is the main document of the leak detector itself. Then it's okay to leak it and it shouldn't be counted as a leak.
,
Nov 8 2016
Looking...
,
Nov 8 2016
My understanding is: * The root cause for the leak has existed before the patch https://codereview.chromium.org/2272773002/, and the actual leak didn't happen at that time * After the patch, the actual leak started to happen and the leak detector detected * The leaked object is ActiveDOMObject, which we don't expect. Is this correct? > Maybe one of the 4 documents is the main document of the leak detector itself. Then it's okay to leak it and it shouldn't be counted as a leak. No, all document objects should be removed after unloading the page.
,
Nov 8 2016
Why was the document leak not detected before the patch?
,
Nov 8 2016
IIUC the actual leak (or a test which causes the leak) didn't exist at that time.
,
Nov 8 2016
> > What I mean is when I run the test locally, I see 4 documents being created and only 3 destroyed, and my IntersectionObserver patch doesn't change these numbers. It increases the number of ActiveDOMObjects which makes the leak detector unhappy. > Maybe one of the 4 documents is the main document of the leak detector itself. Then it's okay to leak it and it shouldn't be counted as a leak. Ah, I was mistaken the context. Yeah, haraken@ might be right. I'm not sure how skyostil@ tested this on his local machine, but the 'leaked' document might be the root document which should exist. The leak detector counts objects in a more strict way. This counts objects after loading another page to remove all document objects in the tested page.
,
Nov 8 2016
,
Nov 10 2016
if i understand lifetimes&dependencies correctly, IntersectionObserverController is considered an extension/supplement of Document, hence weak'ifying the Document reference to it, isn't appropriate. If so, then incrementing https://cs.chromium.org/chromium/src/content/shell/renderer/layout_test/leak_detector.cc?q=kInitialNumberOfLiveActiveDOMObjects&sq=package:chromium&l=27 sounds unavoidable to handle the main document correctly.
,
Nov 11 2016
#22: Ah, so is it the case that IntersectionObserverController is keeping the document alive after it has been constructed? I guess I'm not sure why Oilpan doesn't resolve this cycle, but it could be that we're talking about the root document like hajimehoshi@ mentioned. How does bumping the Active DOM Object count sound to everyone?
,
Nov 11 2016
This is for the root document, which unavoidably will have to stay alive :) So you can't expect some of its members to be GCed somehow before that time. And for the TC in question it wouldn't really help to clear out m_intersectionController during Document detach (in shutdown()).. which isn't currently done, but could be.
,
Nov 11 2016
Thanks for the explanation! I've put together a patch: https://codereview.chromium.org/2496853003/
,
May 30 2017
The current situation is that fast/loader/open-in-srcdoc-unload.html is still leaking:
#LEAK - renderer pid 144923 ({"numberOfLiveSuspendableObjects":[2,3]})
,
May 30 2017
,
May 31 2017
,
May 31 2017
,
May 31 2017
That's strange only a object was leaked but no Document was leaked. Now the leak detector counts objects before and after loading about:blank. Our hypothesis is that document.open loads about:blank to the frame and the leak detector can't treat such situation.
,
May 31 2017
As far as I investigated, when BlinkTestRunner::OnReset() is invoked, a document opening open-in-sercdoc-unload.html is cleared because of document.open(): #READY Document(0x238220d42560)::SetURL(->file://third_party/WebKit/LayoutTests/fast/loader/open-in-srcdoc-unload.html) ... Content-Type: text/plain #EOF #EOF BlinkTestRunner::OnReset() WebLocalFrameImpl::LoadRequest(about:blank) Document(0x238220d42560)::SetURL(file:///third_party/WebKit/LayoutTests/fast/loader/open-in-srcdoc-unload.html->about:blank) #0 0x7fbf7b31f9c7 base::debug::StackTrace::StackTrace() #1 0x7fbf760ac0e0 blink::Document::SetURL() #2 0x7fbf760b6aa6 blink::Document::open() #3 0x7fbf75e6a904 blink::V8Document::openMethodCustom() #4 0x7fbf76870c82 blink::V8Document::openMethodCallback() #5 0x7fbf7701135a v8::internal::FunctionCallbackArguments::Call() #6 0x7fbf770a06c6 v8::internal::(anonymous namespace)::HandleApiCallHelper<>() #7 0x7fbf7709fbef v8::internal::Builtin_Impl_HandleApiCall() #8 0x26fe1530463d <unknown> Document(0x238220d42560)::EnsureIntersectionObserverController(0x254ddd386100) #0 0x7fbf7b31f9c7 base::debug::StackTrace::StackTrace() #1 0x7fbf760c36ca blink::Document::EnsureIntersectionObserverController() #2 0x7fbf760e98c1 blink::ElementIntersectionObserverData::DeactivateAllIntersectionObservers() #3 0x7fbf760dd211 blink::Element::RemovedFrom() #4 0x7fbf762fb22f blink::HTMLIFrameElement::RemovedFrom() #5 0x7fbf76095fff blink::ContainerNode::NotifyNodeRemoved() #6 0x7fbf7609616c blink::ContainerNode::RemoveChildren() #7 0x7fbf760b6b10 blink::Document::ImplicitOpen() #8 0x7fbf760b0afc blink::Document::open() #9 0x7fbf760b6acd blink::Document::open() #10 0x7fbf75e6a904 blink::V8Document::openMethodCustom() #11 0x7fbf76870c82 blink::V8Document::openMethodCallback() #12 0x7fbf7701135a v8::internal::FunctionCallbackArguments::Call() #13 0x7fbf770a06c6 v8::internal::(anonymous namespace)::HandleApiCallHelper<>() #14 0x7fbf7709fbef v8::internal::Builtin_Impl_HandleApiCall() #15 0x26fe1530463d <unknown> WebLocalFrameImpl::LoadRequest(about:blank) - Done BlinkTestRunner::OnReset() - Done ~Document(0x238220d41830) : ~Document(0x238220d433c8) : about:blank ~Document(0x238220d440f8) : about:blank So the document is not destroyed... LeakDetector says that IntersectionObserverController is leaked. # 4 documents are created and 3 Documents are destroyed. We have only 1 document (but the document should be destroyed...) and LeakDetector doesn't say anything about document leak. If we don't have document.open() in unload event listener, EOF #EOF BlinkTestRunner::OnReset() WebLocalFrameImpl::LoadRequest(about:blank) Document(0x3bd16f344f98)::SetURL(->about:blank) // This is not the document opening open-in-srcdoc-unload.html. #0 0x7febc07129c7 base::debug::StackTrace::StackTrace() #1 0x7febbb49f0e0 blink::Document::SetURL() #2 0x7febbb49e6a3 blink::Document::Document() #3 0x7febbb6d5385 blink::HTMLDocument::HTMLDocument() #4 0x7febbb491d4a blink::DOMImplementation::createDocument() #5 0x7febbb651045 blink::LocalDOMWindow::InstallNewDocument() #6 0x7febbba09e3c blink::DocumentLoader::InstallNewDocument() #7 0x7febbba09d22 blink::DocumentLoader::EnsureWriter() #8 0x7febbba090ce blink::DocumentLoader::CommitData() #9 0x7febbba08dde blink::DocumentLoader::FinishedLoading() #10 0x7febbba0a37b blink::DocumentLoader::MaybeLoadEmpty() #11 0x7febbba0a3f6 blink::DocumentLoader::StartLoadingMainResource() #12 0x7febbba19b8e blink::FrameLoader::StartLoad() #13 0x7febbba195d6 blink::FrameLoader::Load() #14 0x7febb9b53032 blink::WebLocalFrameImpl::Load() #15 0x7febb9b5317f blink::WebLocalFrameImpl::LoadRequest() #16 0x00000053a3f9 content::BlinkTestRunner::OnReset() #17 0x00000053a1f5 _ZN3IPC8MessageTI23ShellViewMsg_Reset_MetaSt5tupleIJEEvE8DispatchIN7content15BlinkTestRunnerES7_vMS7_FvvEEEbPKNS_7MessageEPT_PT0_PT1_T2_ #18 0x000000539b6f content::BlinkTestRunner::OnMessageReceived() #19 0x7febbf912982 content::RenderViewImpl::OnMessageReceived() #20 0x7febbdd8ce75 IPC::ChannelProxy::Context::OnDispatchMessage() #21 0x7febc07132e0 base::debug::TaskAnnotator::RunTask() #22 0x7febba822798 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue() #23 0x7febba820ad8 blink::scheduler::TaskQueueManager::DoWork() #24 0x7febc07132e0 base::debug::TaskAnnotator::RunTask() #25 0x7febc0737f8d base::MessageLoop::RunTask() #26 0x7febc07382d8 base::MessageLoop::DeferOrRunPendingTask() #27 0x7febc0738696 base::MessageLoop::DoWork() #28 0x7febc073965a base::MessagePumpDefault::Run() #29 0x7febc0760f5e base::RunLoop::Run() #30 0x7febbf930ddc content::RendererMain() #31 0x7febbfa2c1fa content::RunZygote() #32 0x7febbfa2d088 content::ContentMainRunnerImpl::Run() #33 0x7febb7e53156 service_manager::Main() #34 0x7febbfa2bfe2 content::ContentMain() #35 0x000000472cb1 main #36 0x7febb84bef45 __libc_start_main #37 0x000000472b90 <unknown> Docuemnt(0x3bd16f344f98) WebLocalFrameImpl::LoadRequest(about:blank) - Done BlinkTestRunner::OnReset() - Done ~Document(0x3bd16f341830) : ~Document(0x3bd16f342560) : file:///third_party/WebKit/LayoutTests/fast/loader/open-in-srcdoc-unload.html // This document is destroyed here. So IntersectionObserverController is also destroyed. ~Document(0x3bd16f3433c8) : about:blank ~Document(0x3bd16f3440f8) : about:blank # 5 documents are created and 4 Documents are destroyed.
,
May 31 2017
I think, this is not a real leak. Probably LeakDetector's issue.
,
Jun 1 2017
FYI:
FrameLoader.cpp:
bool FrameLoader::PrepareForCommit() {
...
if (document_loader_) {
Client()->DispatchWillCommitProvisionalLoad();
DispatchUnloadEvent(); // document.open() and document.close() is executed.
}
// The previous calls to dispatchUnloadEvent() and detachChildren() can
// execute arbitrary script via things like unload events. If the executed
// script intiates a new load or causes the current frame to be detached, we
// need to abandon the current load.
if (pdl != provisional_document_loader_)
return false;
I think, "the executed script initiates a new load or causes the current frame to be detached".
So the navigation (to be about:blank) requested by LeakDetector is abandoned.
,
Jun 2 2017
,
Jun 16 2017
hajimehoshi@, do you still own the issue? I'm confused because you reset the status but kept the owner field.
,
Jun 19 2017
Sorry but I no longer own this issue.
,
Jun 27 2017
Sorry, so what's the resolution here? If it's not a real leak would we be able to close it, or do we want more investigation?
,
Jun 27 2017
,
Jun 27 2017
The current situation is that document.open in an unload event tricks the leak detector. There is an issue 583586 that document.open in an unload event will be ignored. After this issue is fixed, the test will become invalidated, so I'd like to wait for that.
,
Jun 27 2017
https://chromium-review.googlesource.com/c/547403/3 I have confirmed that the above CL fixes the issues on win(7) bots. +danakj@: Is the above fix valid? I found |shared_bitmap| can be only when 1. GPU is disabled (on win 7 bots, gpu doesn't work? I'm not sure) 2. Resource is created without a shared bitmap (this can happen when software rendering is used, and shared bitmap is set at ResourceProvider::LockForRead) 3. ResourceProvider::OnMemoryDump is called before ResourceProvider::LockForRead is called These are just guessing. I couldn't reproduce this on my local machines.
,
Jun 27 2017
That sounds right, left you a question on the CL.
,
Jun 27 2017
> 2. Resource is created without a shared bitmap (this can happen when software rendering is used, and shared bitmap is set at ResourceProvider::LockForRead) This means the resource was made with a SharedBitmapId instead.
,
Jun 28 2017
Aaaah, sorry, #40 is for crbug/735173, not here.
,
Jun 28 2017
,
Jun 28 2017
,
Jul 25 2017
It sounds like no action is needed on this bug at the moment, but we want to leave it open for tracking? If so, hajimehoshi, would you mind being the placeholder owner until such time as 583586 actually renders this issue obsolete?
,
Jul 26 2017
No problem.
,
Jan 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/34181304bd765bbd22f621fd3834cc1cd9881e06 commit 34181304bd765bbd22f621fd3834cc1cd9881e06 Author: Dave Tapuska <dtapuska@chromium.org> Date: Mon Jan 14 19:39:59 2019 Remove tests expectations for PausableObjects that no longer fail BUG=907125,661182,506754 Change-Id: I511b0f8f35d6030b7b9609432dc13f9351846c19 Reviewed-on: https://chromium-review.googlesource.com/c/1409483 Reviewed-by: Mustaq Ahmed <mustaq@chromium.org> Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Cr-Commit-Position: refs/heads/master@{#622551} [modify] https://crrev.com/34181304bd765bbd22f621fd3834cc1cd9881e06/third_party/blink/web_tests/LeakExpectations |
||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||
Comment 1 by skyos...@chromium.org
, Nov 1 2016