Bug 115917 - svg/as-image/img-zoom-svg-stylesheet.html crashes with NetworkProcess enabled
Summary: svg/as-image/img-zoom-svg-stylesheet.html crashes with NetworkProcess enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-05-10 10:35 PDT by Brady Eidson
Modified: 2013-05-16 14:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch v1 (6.63 KB, patch)
2013-05-10 10:59 PDT, Brady Eidson
beidson: review-
Details | Formatted Diff | Diff
Patch v2 - Better (5.25 KB, patch)
2013-05-15 17:12 PDT, Brady Eidson
sam: review-
Details | Formatted Diff | Diff
Patch v3 - v2 plus review feedback (7.80 KB, patch)
2013-05-16 09:53 PDT, Brady Eidson
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2013-05-10 10:35:13 PDT
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>
Comment 1 Brady Eidson 2013-05-10 10:59:54 PDT
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
Comment 2 Alexey Proskuryakov 2013-05-10 11:58:21 PDT
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?
Comment 3 Brady Eidson 2013-05-10 12:08:10 PDT
(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.
Comment 4 Alexey Proskuryakov 2013-05-10 12:09:31 PDT
> 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.
Comment 5 Brady Eidson 2013-05-10 20:40:36 PDT
(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.
Comment 6 Brady Eidson 2013-05-15 16:38:34 PDT
Doing this differently.
Comment 7 Brady Eidson 2013-05-15 17:12:47 PDT
Created attachment 201897 [details]
Patch v2 - Better
Comment 8 Sam Weinig 2013-05-15 18:05:37 PDT
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.
Comment 9 Brady Eidson 2013-05-16 09:44:44 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=116233 to get rid of EmptyFrameLoaderClient
Comment 10 Brady Eidson 2013-05-16 09:53:08 PDT
Created attachment 201966 [details]
Patch v3 - v2 plus review feedback
Comment 11 Alexey Proskuryakov 2013-05-16 13:13:48 PDT
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.
Comment 12 Brady Eidson 2013-05-16 13:19:21 PDT
(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.
Comment 13 Brady Eidson 2013-05-16 14:01:49 PDT
http://trac.webkit.org/changeset/150206
Comment 14 Alexey Proskuryakov 2013-05-16 14:52:43 PDT
> 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.