Bug 55017 - SVGImage causes MainResourceLoader leaks
Summary: SVGImage causes MainResourceLoader leaks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Nobody
URL:
Keywords:
Depends on: 55185
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-22 19:58 PST by Xianzhu Wang
Modified: 2011-03-04 23:08 PST (History)
8 users (show)

See Also:


Attachments
patch (6.01 KB, patch)
2011-02-23 02:16 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
updated patch (5.85 KB, patch)
2011-02-23 04:54 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch using loader->init() instead of loader->load(fakeRequest) (6.01 KB, patch)
2011-02-23 19:28 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Update patch to fix assertion failures (6.11 KB, patch)
2011-02-24 20:36 PST, Xianzhu Wang
abarth: review-
Details | Formatted Diff | Diff
new patch (5.54 KB, patch)
2011-03-02 18:53 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Fix style (5.54 KB, patch)
2011-03-02 19:02 PST, Xianzhu Wang
abarth: review-
Details | Formatted Diff | Diff
Fix sandbox issue (5.54 KB, patch)
2011-03-03 01:31 PST, Xianzhu Wang
abarth: review+
Details | Formatted Diff | Diff
Final patch (5.58 KB, patch)
2011-03-03 21:14 PST, Xianzhu Wang
wangxianzhu: commit-queue-
Details | Formatted Diff | Diff
Really final:) Fixed reviewer in LayoutTests/ChangeLog (5.58 KB, patch)
2011-03-04 15:41 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 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.
Comment 1 Xianzhu Wang 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.
Comment 2 Xianzhu Wang 2011-02-23 02:16:58 PST
Created attachment 83459 [details]
patch
Comment 3 Johnny(Jianning) Ding 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>'
Comment 4 Xianzhu Wang 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.
Comment 5 Xianzhu Wang 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.
Comment 6 Alexey Proskuryakov 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()));
Comment 7 Xianzhu Wang 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.
Comment 8 Xianzhu Wang 2011-02-23 19:28:15 PST
Created attachment 83597 [details]
patch using loader->init() instead of loader->load(fakeRequest)
Comment 9 Alexey Proskuryakov 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.
Comment 10 Adam Barth 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.
Comment 11 WebKit Commit Bot 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>
Comment 12 WebKit Commit Bot 2011-02-24 12:58:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 James Robinson 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
Comment 14 James Robinson 2011-02-24 14:15:55 PST
Committed r79620: <http://trac.webkit.org/changeset/79620>
Comment 15 James Robinson 2011-02-24 14:16:14 PST
That assertion tripped on:
compositing/images/direct-svg-image.html
fast/canvas/svg-taint.html
Comment 16 WebKit Review Bot 2011-02-24 16:07:31 PST
http://trac.webkit.org/changeset/79620 might have broken GTK Linux 64-bit Debug
Comment 17 Xianzhu Wang 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?
Comment 18 Xianzhu Wang 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.
Comment 19 Xianzhu Wang 2011-02-24 20:36:05 PST
Created attachment 83766 [details]
Update patch to fix assertion failures
Comment 20 Xianzhu Wang 2011-03-01 15:40:48 PST
Hi, Adam, could you please take a look at the updated patch?
Comment 21 Adam Barth 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.
Comment 22 Xianzhu Wang 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.
Comment 23 Xianzhu Wang 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
Comment 24 WebKit Review Bot 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.
Comment 25 Xianzhu Wang 2011-03-02 19:02:32 PST
Created attachment 84509 [details]
Fix style
Comment 26 Adam Barth 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.
Comment 27 Adam Barth 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.  :)
Comment 28 Xianzhu Wang 2011-03-03 01:31:44 PST
Created attachment 84534 [details]
Fix sandbox issue

Sorry. My fault. Fixed.
Comment 29 Alexey Proskuryakov 2011-03-03 09:19:58 PST
Does this need an additional test case to cover the sandbox mistake?
Comment 30 Adam Barth 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.
Comment 31 Adam Barth 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.
Comment 32 Alexey Proskuryakov 2011-03-03 13:28:45 PST
I also don't know how to test this. Thanks!
Comment 33 Xianzhu Wang 2011-03-03 21:14:22 PST
Created attachment 84686 [details]
Final patch

Thanks Adam for review!
Comment 34 WebKit Commit Bot 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.
Comment 35 WebKit Commit Bot 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
Comment 36 Eric Seidel (no email) 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.
Comment 37 Xianzhu Wang 2011-03-04 15:41:37 PST
Created attachment 84818 [details]
Really final:) Fixed reviewer in LayoutTests/ChangeLog
Comment 38 WebKit Commit Bot 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>
Comment 39 WebKit Commit Bot 2011-03-04 23:08:52 PST
All reviewed patches have been landed.  Closing bug.