Bug 55017 – SVGImage causes MainResourceLoader leaks

• Component: Page Loading
• Status: RESOLVED FIXED

[Xianzhu Wang]

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.

[Xianzhu Wang]

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]

Created attachment 83459 [details]
patch

[Johnny(Jianning) Ding]

(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]

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]

(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 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]

(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]

Created attachment 83597 [details]
patch using loader->init() instead of loader->load(fakeRequest)

[Alexey Proskuryakov]

> >     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 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 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]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

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]

Committed r79620: <http://trac.webkit.org/changeset/79620>

[James Robinson]

That assertion tripped on:
compositing/images/direct-svg-image.html
fast/canvas/svg-taint.html

[WebKit Review Bot]

http://trac.webkit.org/changeset/79620 might have broken GTK Linux 64-bit Debug

[Xianzhu Wang]

(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]

(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]

Created attachment 83766 [details]
Update patch to fix assertion failures

[Xianzhu Wang]

Hi, Adam, could you please take a look at the updated patch?

[Adam Barth]

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]

(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]

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]

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]

Created attachment 84509 [details]
Fix style

[Adam Barth]

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 on attachment 84509 [details]
Fix style

I should say that the rest of the change looks good.  :)

[Xianzhu Wang]

Created attachment 84534 [details]
Fix sandbox issue

Sorry. My fault. Fixed.

[Alexey Proskuryakov]

Does this need an additional test case to cover the sandbox mistake?

[Adam Barth]

(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 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]

I also don't know how to test this. Thanks!

[Xianzhu Wang]

Created attachment 84686 [details]
Final patch

Thanks Adam for review!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 84686 [details]:

inspector/audits/audits-panel-functional.html bug 55776 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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)]

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]

Created attachment 84818 [details]
Really final:) Fixed reviewer in LayoutTests/ChangeLog

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.