| Summary: | SVGImage causes MainResourceLoader leaks | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> | ||||||||||||||||||||
| Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Major | CC: | abarth, ap, commit-queue, eric, jamesr, japhet, jnd, webkit.review.bot | ||||||||||||||||||||
| Priority: | P1 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Bug Depends on: | 55185 | ||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
Sorry for typos
> didFinishLoading() of the newly created ResourceLoader will be never called, causing the ResourceLoader stay in the list of loading loaders list of ResourceLoaderScheduler forever. When the number of leaked ResourceLoaderScheduler reaches 20, no more new loaders will be executed and the whole browser will stop working.
Should be:
didFinishLoading() of the newly created ResourceLoader will be never called, causing the ResourceLoader stay in the list of loading loaders of ResourceLoadScheduler forever. When the number of leaked ResourceLoaders reaches 20, no more new loaders will be executed and the whole browser will stop working.
Created attachment 83459 [details]
patch
(In reply to comment #2) > Created an attachment (id=83459) [details] > patch Nice, one suggestion, in the layout test, using the following data URL may save two more files. scriptFrame.src = 'data:text/html;charset=utf-8,<script src="empty.js"></script>' Created attachment 83472 [details]
updated patch
I've tried data URL, but it didn't work:
scriptFrame.src = "data:text/html,<script src=x.js><" + "</script>"
won't load x.js at all (may be a security restriction?)
So I still use an external html for the frame.
Adopted Johnny's another suggestion, removed blank.js and use XXXXX.js in load-script.html.
The issue can be triggered no matter the resource exists or not.
This updated patch also fixed a problem in the test case: "totalFrames = 10" was wrong because I forgot to revert a temporary change.
(In reply to comment #4) > scriptFrame.src = "data:text/html,<script src=x.js><" + "</script>" scriptFrame.src = "data:text/html,<script src=x.js><" + "/script>", in fact. Comment on attachment 83472 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=83472&action=review Nice catch. > Source/WebCore/svg/graphics/SVGImage.cpp:285 > + loader->activeDocumentLoader()->cancelMainResourceLoad(ResourceError()); Will this result in an error line in Safari activity window, and/or in Web Inspector? FWIW, existing callers of this function provide a real error object. The case in WebHTMLRepresentation.mm looks very similar: if (coreFrame->document()->isMediaDocument()) coreFrame->loader()->documentLoader()->cancelMainResourceLoad(coreFrame->loader()->client()->pluginWillHandleLoadError(coreFrame->loader()->documentLoader()->response())); (In reply to comment #6) > (From update of attachment 83472 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83472&action=review > > Nice catch. > > > Source/WebCore/svg/graphics/SVGImage.cpp:285 > > + loader->activeDocumentLoader()->cancelMainResourceLoad(ResourceError()); > > Will this result in an error line in Safari activity window, and/or in Web Inspector? > Seems the Web Inspector will report this as an error. > FWIW, existing callers of this function provide a real error object. The case in WebHTMLRepresentation.mm looks very similar: > > if (coreFrame->document()->isMediaDocument()) > coreFrame->loader()->documentLoader()->cancelMainResourceLoad(coreFrame->loader()->client()->pluginWillHandleLoadError(coreFrame->loader()->documentLoader()->response())); This is not a real error, just a cancellation of the fake empty request with no error. ResourceLoader will replace the empty error with a cancellation error. I'm wondering better ways to avoid the leak. 1) better way to clear the loader for the fake request. frame loader -> document loader -> main resource loader (In reply to comment #6) > (From update of attachment 83472 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83472&action=review > > Nice catch. > > > Source/WebCore/svg/graphics/SVGImage.cpp:285 > > + loader->activeDocumentLoader()->cancelMainResourceLoad(ResourceError()); > > Will this result in an error line in Safari activity window, and/or in Web Inspector? > > FWIW, existing callers of this function provide a real error object. The case in WebHTMLRepresentation.mm looks very similar: > > if (coreFrame->document()->isMediaDocument()) > coreFrame->loader()->documentLoader()->cancelMainResourceLoad(coreFrame->loader()->client()->pluginWillHandleLoadError(coreFrame->loader()->documentLoader()->response())); There is no real error because I just want to cancel the loading for the fake request. ResourceLoader will replace the empty ResourceError with a cancellation error. However, I just found a way to avoid calling loader->load(...). I'll create a patch soon. Created attachment 83597 [details]
patch using loader->init() instead of loader->load(fakeRequest)
> > if (coreFrame->document()->isMediaDocument())
> > coreFrame->loader()->documentLoader()->cancelMainResourceLoad(coreFrame->loader()->client()->pluginWillHandleLoadError(coreFrame->loader()->documentLoader()->response()));
>
> There is no real error because I just want to cancel the loading for the fake request.
This doesn't matter any more, but it's the same in the example I gave. It's not an error, but a non-null error object is provided nonetheless.
Comment on attachment 83597 [details]
patch using loader->init() instead of loader->load(fakeRequest)
I'm not sure this is 100% correct (or even if this code could even be correct), but calling init is much better than trying a fake load.
Comment on attachment 83597 [details] patch using loader->init() instead of loader->load(fakeRequest) Clearing flags on attachment: 83597 Committed r79604: <http://trac.webkit.org/changeset/79604> All reviewed patches have been landed. Closing bug. This is causing some assertion failures, for example: ASSERTION FAILED: State(m_state + 1) == state || (firstLayoutDone() && state == CommittedFirstRealLoad) third_party/WebKit/Source/WebCore/loader/FrameLoaderStateMachine.cpp(69) : void WebCore::FrameLoaderStateMachine::advanceTo(WebCore::FrameLoaderStateMachine::State) [19278:19278:3100559591770:ERROR:process_util_posix.cc(107)] Received signal 11 on compositing/images/direct-svg-image.html Committed r79620: <http://trac.webkit.org/changeset/79620> That assertion tripped on: compositing/images/direct-svg-image.html fast/canvas/svg-taint.html http://trac.webkit.org/changeset/79620 might have broken GTK Linux 64-bit Debug (In reply to comment #15) > That assertion tripped on: > compositing/images/direct-svg-image.html > fast/canvas/svg-taint.html I can't reproduce the assertion failure with chromium-linux debug version. Which platform are you using? (In reply to comment #17) > (In reply to comment #15) > > That assertion tripped on: > > compositing/images/direct-svg-image.html > > fast/canvas/svg-taint.html > > I can't reproduce the assertion failure with chromium-linux debug version. > Which platform are you using? Sorry I used the wrong version. Please ignore the above reply. Reopening the bug because the patch was rolled out. Created attachment 83766 [details]
Update patch to fix assertion failures
Hi, Adam, could you please take a look at the updated patch? Comment on attachment 83766 [details] Update patch to fix assertion failures View in context: https://bugs.webkit.org/attachment.cgi?id=83766&action=review > Source/WebCore/svg/graphics/SVGImage.cpp:279 > + if (!loader->activeDocumentLoader()) > + loader->init(); // Make sure the DocumentLoader is created Why is there a branch here? This boot-up process should be deterministic. (In reply to comment #21) > (From update of attachment 83766 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83766&action=review > > > Source/WebCore/svg/graphics/SVGImage.cpp:279 > > + if (!loader->activeDocumentLoader()) > > + loader->init(); // Make sure the DocumentLoader is created > > Why is there a branch here? This boot-up process should be deterministic. After comparing the execution paths on chromium-linux and mac, I found I was misled by the execution path on chromium-linux: the code is executed for the first image contained in an SVG document, which seems incorrect. SVGImage shouldn't be created and loaded at all for the tests because no SVG document is used as an embedded image. The execution path doesn't happen on mac webkit. However, that SVGImage causes MainResourceLoader leaks is still a fact on all platforms thought it should be triggered with different tests. The tests listed in my first bug description just accidentally triggered this issue on chromium-linux (maybe also some other platforms). I will file another bug for the wrong execution path issue on chromium-linux, and create a new test case for this issue. Created attachment 84507 [details] new patch Hi, Adam, This new patch removed the loader->init() code which is unnecessary because frame->init() has already initialized the loader. Also updated the test case suitable for all platforms. Could you please take another look? About chromium-linux's unnecessary SVGImage creation, I created bug 55643 to track it. Thanks, Xianzhu Attachment 84507 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/svg/graphics/SVGImage.cpp:277: One space before end of line comments [whitespace/comments] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 84509 [details]
Fix style
Comment on attachment 84509 [details] Fix style View in context: https://bugs.webkit.org/attachment.cgi?id=84509&action=review > Source/WebCore/svg/graphics/SVGImage.cpp:-278 > - loader->setForcedSandboxFlags(SandboxAll); Why did you remove this sandboxing? We still want this. Comment on attachment 84509 [details]
Fix style
I should say that the rest of the change looks good. :)
Created attachment 84534 [details]
Fix sandbox issue
Sorry. My fault. Fixed.
Does this need an additional test case to cover the sandbox mistake? (In reply to comment #29) > Does this need an additional test case to cover the sandbox mistake? I could be wrong, but I don't think the sandbox bits are observable here. We're using them as defense in depth. The problem is you can't run script inside an SVGImage, which makes it tough to poke at things and see if you're sandboxed. I'm open to suggestions if you have a clever idea how to test it. Comment on attachment 84534 [details] Fix sandbox issue View in context: https://bugs.webkit.org/attachment.cgi?id=84534&action=review I continue to like this patch because it removes cruft from the SVGImage bootup code. Please give Alexey a chance to suggest an approach for additional testing before landing. Thanks! > LayoutTests/fast/images/svg-image-leak-loader.html:6 > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); We usually use four-space indent in tests, but it doesn't really matter. > LayoutTests/fast/images/resources/load-script.html:3 > +<script src="XXXXX.js"></script> I might have called this "does-not-exist.js" so a future reader doesn't need to wonder what XXXXX means. I also don't know how to test this. Thanks! Created attachment 84686 [details]
Final patch
Thanks Adam for review!
The commit-queue encountered the following flaky tests while processing attachment 84686 [details]: inspector/audits/audits-panel-functional.html bug 55776 (authors: apavlov@chromium.org, pfeldman@chromium.org, and rniwa@webkit.org) The commit-queue is continuing to process your patch. Comment on attachment 84686 [details] Final patch Rejecting attachment 84686 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 1 Last 500 characters of output: ages.in M Source/WebKit2/WebProcess/WebPage/WebPage.cpp M Tools/TestWebKitAPI/JavaScriptTest.cpp M Tools/TestWebKitAPI/Tests/WebKit2/PageLoadDidChangeLocationWithinPageForFrame.cpp M Tools/TestWebKitAPI/Tests/WebKit2/PreventEmptyUserAgent.cpp M Tools/TestWebKitAPI/Tests/WebKit2/EvaluateJavaScript.cpp M Tools/ChangeLog r80358 = da0785b77f5fe28405a577a7d2bb2afb8a665b8c (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/8082727 NOBODY (OOPS!) found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py. was the error. Created attachment 84818 [details]
Really final:) Fixed reviewer in LayoutTests/ChangeLog
Comment on attachment 84818 [details] Really final:) Fixed reviewer in LayoutTests/ChangeLog Clearing flags on attachment: 84818 Committed r80410: <http://trac.webkit.org/changeset/80410> All reviewed patches have been landed. Closing bug. |
The following code (in WebCore/svg/graphics/SVGImage.cpp) causes ResourceLoader leak: RefPtr<Frame> frame = Frame::create(m_page.get(), 0, dummyFrameLoaderClient); frame->setView(FrameView::create(frame.get())); frame->init(); ResourceRequest fakeRequest(KURL(ParsedURLString, "")); FrameLoader* loader = frame->loader(); loader->setForcedSandboxFlags(SandboxAll); loader->load(fakeRequest, false); // Make sure the DocumentLoader is created didFinishLoading() of the newly created ResourceLoader will be never called, causing the ResourceLoader stay in the list of loading loaders list of ResourceLoaderScheduler forever. When the number of leaked ResourceLoaderScheduler reaches 20, no more new loaders will be executed and the whole browser will stop working. I reproduced the issue in chromium-linux environment running the following layout tests with single process mode (new-run-webkit-tests --no-retry-failures --no-pixel-tests --child-processes=1 --test-list=a_file_containing_the_following_lines): svg/W3C-SVG-1.1/struct-use-01-t.svg svg/W3C-SVG-1.1/struct-image-07-t.svg svg/W3C-SVG-1.1/struct-image-05-b.svg svg/W3C-SVG-1.1/struct-image-05-b.svg svg/W3C-SVG-1.1/filters-light-01-f.svg svg/W3C-SVG-1.1/color-prof-01-f.svg svg/W3C-SVG-1.1/filters-diffuse-01-f.svg svg/W3C-SVG-1.1/struct-image-03-t.svg svg/W3C-SVG-1.1/struct-image-10-t.svg svg/W3C-SVG-1.1/struct-symbol-01-b.svg svg/W3C-SVG-1.1/struct-image-02-b.svg svg/W3C-SVG-1.1/struct-image-06-t.svg svg/W3C-SVG-1.1/struct-image-01-t.svg svg/W3C-SVG-1.1/filters-conv-01-f.svg svg/W3C-SVG-1.1/render-groups-03-t.svg svg/W3C-SVG-1.1/struct-image-09-t.svg svg/W3C-SVG-1.1/render-groups-01-b.svg svg/W3C-SVG-1.1/struct-image-08-t.svg svg/W3C-SVG-1.1/masking-path-04-b.svg svg/W3C-SVG-1.1/filters-specular-01-f.svg svg/animations/animVal-basics.html The last test will timeout. I'll provide a patch today.