RESOLVED FIXED 55017
SVGImage causes MainResourceLoader leaks
https://bugs.webkit.org/show_bug.cgi?id=55017
Summary SVGImage causes MainResourceLoader leaks
Xianzhu Wang
Reported 2011-02-22 19:58:02 PST
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.
Attachments
patch (6.01 KB, patch)
2011-02-23 02:16 PST, Xianzhu Wang
no flags
updated patch (5.85 KB, patch)
2011-02-23 04:54 PST, Xianzhu Wang
no flags
patch using loader->init() instead of loader->load(fakeRequest) (6.01 KB, patch)
2011-02-23 19:28 PST, Xianzhu Wang
no flags
Update patch to fix assertion failures (6.11 KB, patch)
2011-02-24 20:36 PST, Xianzhu Wang
abarth: review-
new patch (5.54 KB, patch)
2011-03-02 18:53 PST, Xianzhu Wang
no flags
Fix style (5.54 KB, patch)
2011-03-02 19:02 PST, Xianzhu Wang
abarth: review-
Fix sandbox issue (5.54 KB, patch)
2011-03-03 01:31 PST, Xianzhu Wang
abarth: review+
Final patch (5.58 KB, patch)
2011-03-03 21:14 PST, Xianzhu Wang
wangxianzhu: commit-queue-
Really final:) Fixed reviewer in LayoutTests/ChangeLog (5.58 KB, patch)
2011-03-04 15:41 PST, Xianzhu Wang
no flags
Xianzhu Wang
Comment 1 2011-02-22 20:07:01 PST
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.
Xianzhu Wang
Comment 2 2011-02-23 02:16:58 PST
Johnny(Jianning) Ding
Comment 3 2011-02-23 03:57:58 PST
(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>'
Xianzhu Wang
Comment 4 2011-02-23 04:54:30 PST
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.
Xianzhu Wang
Comment 5 2011-02-23 04:56:39 PST
(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.
Alexey Proskuryakov
Comment 6 2011-02-23 08:56:37 PST
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()));
Xianzhu Wang
Comment 7 2011-02-23 19:12:29 PST
(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.
Xianzhu Wang
Comment 8 2011-02-23 19:28:15 PST
Created attachment 83597 [details] patch using loader->init() instead of loader->load(fakeRequest)
Alexey Proskuryakov
Comment 9 2011-02-23 19:37:58 PST
> > 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.
Adam Barth
Comment 10 2011-02-24 02:37:17 PST
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.
WebKit Commit Bot
Comment 11 2011-02-24 12:58:01 PST
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>
WebKit Commit Bot
Comment 12 2011-02-24 12:58:05 PST
All reviewed patches have been landed. Closing bug.
James Robinson
Comment 13 2011-02-24 14:13:11 PST
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
James Robinson
Comment 14 2011-02-24 14:15:55 PST
James Robinson
Comment 15 2011-02-24 14:16:14 PST
That assertion tripped on: compositing/images/direct-svg-image.html fast/canvas/svg-taint.html
WebKit Review Bot
Comment 16 2011-02-24 16:07:31 PST
http://trac.webkit.org/changeset/79620 might have broken GTK Linux 64-bit Debug
Xianzhu Wang
Comment 17 2011-02-24 19:53:17 PST
(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?
Xianzhu Wang
Comment 18 2011-02-24 20:12:49 PST
(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.
Xianzhu Wang
Comment 19 2011-02-24 20:36:05 PST
Created attachment 83766 [details] Update patch to fix assertion failures
Xianzhu Wang
Comment 20 2011-03-01 15:40:48 PST
Hi, Adam, could you please take a look at the updated patch?
Adam Barth
Comment 21 2011-03-01 16:16:34 PST
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.
Xianzhu Wang
Comment 22 2011-03-02 05:18:25 PST
(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.
Xianzhu Wang
Comment 23 2011-03-02 18:53:50 PST
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
WebKit Review Bot
Comment 24 2011-03-02 18:56:02 PST
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.
Xianzhu Wang
Comment 25 2011-03-02 19:02:32 PST
Created attachment 84509 [details] Fix style
Adam Barth
Comment 26 2011-03-02 22:48:51 PST
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.
Adam Barth
Comment 27 2011-03-02 22:49:21 PST
Comment on attachment 84509 [details] Fix style I should say that the rest of the change looks good. :)
Xianzhu Wang
Comment 28 2011-03-03 01:31:44 PST
Created attachment 84534 [details] Fix sandbox issue Sorry. My fault. Fixed.
Alexey Proskuryakov
Comment 29 2011-03-03 09:19:58 PST
Does this need an additional test case to cover the sandbox mistake?
Adam Barth
Comment 30 2011-03-03 13:00:06 PST
(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.
Adam Barth
Comment 31 2011-03-03 13:07:34 PST
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.
Alexey Proskuryakov
Comment 32 2011-03-03 13:28:45 PST
I also don't know how to test this. Thanks!
Xianzhu Wang
Comment 33 2011-03-03 21:14:22 PST
Created attachment 84686 [details] Final patch Thanks Adam for review!
WebKit Commit Bot
Comment 34 2011-03-04 09:02:52 PST
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.
WebKit Commit Bot
Comment 35 2011-03-04 09:04:36 PST
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
Eric Seidel (no email)
Comment 36 2011-03-04 11:57:13 PST
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.
Xianzhu Wang
Comment 37 2011-03-04 15:41:37 PST
Created attachment 84818 [details] Really final:) Fixed reviewer in LayoutTests/ChangeLog
WebKit Commit Bot
Comment 38 2011-03-04 23:08:45 PST
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>
WebKit Commit Bot
Comment 39 2011-03-04 23:08:52 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.