Bug 115917 – svg/as-image/img-zoom-svg-stylesheet.html crashes with NetworkProcess enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

svg/as-image/img-zoom-svg-stylesheet.html crashes with NetworkProcess enabled

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit2             	0x0000000110073b60 WebKit::WebFrame::page() const + 16 (WebFrame.cpp:176)
1   com.apple.WebKit2             	0x00000001102033c0 WebKit::WebResourceLoadScheduler::scheduleLoad(WebCore::ResourceLoader*, WebCore::CachedResource*, WebCore::ResourceLoadPriority, bool) + 848 (WebResourceLoadScheduler.cpp:109)
2   com.apple.WebKit2             	0x000000011020303a WebKit::WebResourceLoadScheduler::scheduleSubresourceLoad(WebCore::Frame*, WebCore::CachedResource*, WebCore::ResourceRequest const&, WebCore::ResourceLoadPriority, WebCore::ResourceLoaderOptions const&) + 218 (WebResourceLoadScheduler.cpp:73)
3   com.apple.WebCore             	0x00000001127b6d3b WebCore::CachedResource::load(WebCore::CachedResourceLoader*, WebCore::ResourceLoaderOptions const&) + 1579 (CachedResource.cpp:346)
4   com.apple.WebCore             	0x00000001127c3d37 WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&) + 1911 (CachedResourceLoader.cpp:498)
5   com.apple.WebCore             	0x00000001127c4100 WebCore::CachedResourceLoader::requestCSSStyleSheet(WebCore::CachedResourceRequest&) + 64 (CachedResourceLoader.cpp:195)
6   com.apple.WebCore             	0x00000001139be771 WebCore::ProcessingInstruction::checkStyleSheet() + 2193 (ProcessingInstruction.cpp:179)
7   com.apple.WebCore             	0x00000001139bf39b WebCore::ProcessingInstruction::insertedInto(WebCore::ContainerNode*) + 139 (ProcessingInstruction.cpp:301)
8   com.apple.WebCore             	0x0000000112871bf7 WebCore::ChildNodeInsertionNotifier::notifyNodeInsertedIntoDocument(WebCore::Node*) + 167 (ContainerNodeAlgorithms.h:198)
9   com.apple.WebCore             	0x000000011286e69f WebCore::ChildNodeInsertionNotifier::notify(WebCore::Node*) + 207 (ContainerNodeAlgorithms.h:226)
10  com.apple.WebCore             	0x00000001128695d5 WebCore::ContainerNode::parserAppendChild(WTF::PassRefPtr<WebCore::Node>) + 773 (ContainerNode.cpp:713)
11  com.apple.WebCore             	0x00000001141e49e6 WebCore::XMLDocumentParser::processingInstruction(unsigned char const*, unsigned char const*) + 342 (XMLDocumentParserLibxml2.cpp:990)
12  com.apple.WebCore             	0x00000001141e5578 WebCore::processingInstructionHandler(void*, unsigned char const*, unsigned char const*) + 72 (XMLDocumentParserLibxml2.cpp:1134)
...

We're casting an EmptyFrameLoaderClient to a WebFrameLoaderClient.  Yikes.

In radar as <rdar://problem/13837408>

[Brady Eidson]

Created attachment 201362 [details]
Patch v1

This patch is straight forward and makes things much safer in WK2.

I don't feel great about it - It is arguably a layering violation in FrameLoaderClient.  But definitely less of one than "hasWebView" is, for example.

That said, I can't think of a fix for this problem that:
A - Isn't much more hackey (exposing an "isEmptyFrameLoaderClient() method, for example)
B - Doesn't require much more refactoring of Frame/FrameLoader/Page (pushing ID's down in to WebCore)
C - Doesn't require much more refactoring of SVG-as-images (making them use the normal WebFrame mechanism instead of directly creating their own WebCore::Frame

[Alexey Proskuryakov]

Comment on attachment 201362 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=201362&action=review

> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:-108
> -    WebFrame* webFrame = static_cast<WebFrameLoaderClient*>(resourceLoader->frameLoader()->client())->webFrame();

We have a lot of such static_casts in WebKit2.

Can we perhaps have a HashMap<FrameLoaderClient*, WebFrameLoaderClient*>, and replace all such casts with a map lookup at once?

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 201362 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201362&action=review
> 
> > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:-108
> > -    WebFrame* webFrame = static_cast<WebFrameLoaderClient*>(resourceLoader->frameLoader()->client())->webFrame();
> 
> We have a lot of such static_casts in WebKit2.
> 
> Can we perhaps have a HashMap<FrameLoaderClient*, WebFrameLoaderClient*>, and replace all such casts with a map lookup at once?

Yikes.

[Alexey Proskuryakov]

> A - Isn't much more hackey (exposing an "isEmptyFrameLoaderClient() method, for example)

It would not necessarily be horrible if WebCore exposed some way for WebKit to know that a frame it's talking about is one created by WebKit. For example, what if we had FrameLoader::clientFrame(), which would be safe for WebCore client to cast.

Something like:

- WebFrame* webFrame = static_cast<WebFrameLoaderClient*>(resourceLoader->frameLoader()->client())->webFrame();
+ WebFrame* webFrame = static_cast<WebFrame*>(resourceLoader->frameLoader()->clientFrame());

Not quite sure about the name though.

[Brady Eidson]

(In reply to comment #4)
> > A - Isn't much more hackey (exposing an "isEmptyFrameLoaderClient() method, for example)
> 
> It would not necessarily be horrible if WebCore exposed some way for WebKit to know that a frame it's talking about is one created by WebKit. For example, what if we had FrameLoader::clientFrame(), which would be safe for WebCore client to cast.
> 
> Something like:
> 
> - WebFrame* webFrame = static_cast<WebFrameLoaderClient*>(resourceLoader->frameLoader()->client())->webFrame();
> + WebFrame* webFrame = static_cast<WebFrame*>(resourceLoader->frameLoader()->clientFrame());
> 
> Not quite sure about the name though.

I like this idea.

I'm okay with clientFrame.  If you're not convinced, I could suggest:
::clientProvidedFrame
::clientOwnedFrame
::clientManagedFrame
::clientData
::clientFrameData
::embedderFrame
::platformFrame
...
or variants thereof.

[Brady Eidson]

Doing this differently.

[Brady Eidson]

Created attachment 201897 [details]
Patch v2 - Better

[Sam Weinig]

Comment on attachment 201897 [details]
Patch v2 - Better

View in context: https://bugs.webkit.org/attachment.cgi?id=201897&action=review

> Source/WebCore/loader/FrameLoaderClient.h:352
> +        virtual bool isEmptyFrameLoaderClient() { return false; }

Please add a FIXME here with a bug to remove it and EmprtFrameLoaderClient, which is an abomination.

> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:120
> +    loadParameters.webPageID = webPage ? webPage->pageID() : 0;

This seems dangerous.  Please add assertions that these are not misused.

[Brady Eidson]

Filed https://bugs.webkit.org/show_bug.cgi?id=116233 to get rid of EmptyFrameLoaderClient

[Brady Eidson]

Created attachment 201966 [details]
Patch v3 - v2 plus review feedback

[Alexey Proskuryakov]

Comment on attachment 201966 [details]
Patch v3 - v2 plus review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=201966&action=review

> Source/WebCore/ChangeLog:10
> +        No new tests (Covered by existing test).

I'd just omit this line, it should be obvious from bug title.

> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:114
> +    if (!resourceLoader->frameLoader()->client()->isEmptyFrameLoaderClient()) {
> +        webFrame = static_cast<WebFrameLoaderClient*>(resourceLoader->frameLoader()->client())->webFrame();

I would have added a "toWebFrameLoaderClient" function, and replaced all static casts with it.

In WebKit1, too.

[Brady Eidson]

(In reply to comment #11)
> (From update of attachment 201966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201966&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests (Covered by existing test).
> 
> I'd just omit this line, it should be obvious from bug title.

Makes sense.

> > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:114
> > +    if (!resourceLoader->frameLoader()->client()->isEmptyFrameLoaderClient()) {
> > +        webFrame = static_cast<WebFrameLoaderClient*>(resourceLoader->frameLoader()->client())->webFrame();
> 
> I would have added a "toWebFrameLoaderClient" function, and replaced all static casts with it.
> 
> In WebKit1, too.

I think a patch 30x larger is not necessary to fix the only specific known symptom (this crash) in one specific code site (here in WebResourceLoadScheduler)

That said, I agree that all such casts need to be given a once-over.  Unfortunately since that function could return null the task would be much larger than a simple global replace.  Decision making would have to take place at each call site.

[Brady Eidson]

http://trac.webkit.org/changeset/150206

[Alexey Proskuryakov]

> Unfortunately since that function could return null the task would be much larger than a simple global replace.

Replacing a security sensitive crash with a null pointer crash could be done automatically.