Bug 40742

Summary: WebCore crashes when removing a link element in a beforeload handler
Product: WebKit Reporter: Andy Estes <aestes>
Component: Page LoadingAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Major CC: aestes, ap, dglazkov, eric
Priority: P1 Keywords: HasReduction, InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 43813    
Attachments:
Description Flags
test case
none
Patch mitz: review+, aestes: commit-queue-

Description Andy Estes 2010-06-16 15:34:40 PDT
Created attachment 58941 [details]
test case

If a page has a beforeload handler that removes a stylesheet <link> element, WebCore will crash with the following backtrace:

Crash is in WebCore::Node::createRendererIfNeeded:

0   com.apple.WebCore             	0x00007fff8808d6bd WebCore::Node::createRendererIfNeeded() + 45
1   com.apple.WebCore             	0x00007fff8808d5a0 WebCore::Element::attach() + 32
2   com.apple.WebCore             	0x00007fff8808cbb1 WebCore::HTMLParser::insertNode(WebCore::Node*, bool) + 369
3   com.apple.WebCore             	0x00007fff88110fd4 WebCore::HTMLParser::parseToken(WebCore::Token*) + 868
4   com.apple.WebCore             	0x00007fff8808c621 WebCore::HTMLTokenizer::processToken() + 657
5   com.apple.WebCore             	0x00007fff8810d786 WebCore::HTMLTokenizer::parseTag(WebCore::SegmentedString&, WebCore::HTMLTokenizer::State) + 4950
6   com.apple.WebCore             	0x00007fff8810bc60 WebCore::HTMLTokenizer::write(WebCore::SegmentedString const&, bool) + 2720
7   com.apple.WebCore             	0x00007fff8826607e WebCore::HTMLTokenizer::executeExternalScriptsIfReady() + 1694
8   com.apple.WebCore             	0x00007fff882147fc WebCore::CachedScript::checkNotify() + 76
9   com.apple.WebCore             	0x00007fff881e9226 WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader*) + 358
10  com.apple.WebCore             	0x00007fff881e9021 WebCore::SubresourceLoader::didFinishLoading() + 49

The attached .html file demonstrates the crash.
Comment 1 Andy Estes 2010-06-16 15:34:56 PDT
<rdar://problem/8091385>
Comment 2 Andy Estes 2010-06-18 00:30:21 PDT
Created attachment 59074 [details]
Patch
Comment 3 mitz 2010-06-18 00:45:57 PDT
Comment on attachment 59074 [details]
Patch

> +        Postpone loading of link elements until after they have been inserted into the DOM and
> +        attached. This prevents mutation events triggered by beforeload handlers from firing in the
> +        midst of DOM insertion, which can lead to assertion failures and crashes.

Is the problem here really the mutation events, or merely the fact that the beforeload handler mutates the DOM during insertion of the link element?

> +void HTMLLinkElement::processCallback(Node* n)
> +{
> +    static_cast<HTMLLinkElement*>(n)->process();
> +}

I would assert (or ASSERT_ARG) that n is a link element before casting it.

> +	<link rel="stylesheet" href="">

href="" means that the base URL (the .html file in this case) will be loaded as the stylesheet in this case. Not a big deal, but perhaps href="data:text/css," is cleaner (if it doesn’t invalidate the test).
Comment 4 Andy Estes 2010-06-18 00:49:11 PDT
(In reply to comment #3)
> (From update of attachment 59074 [details])
> > +        Postpone loading of link elements until after they have been inserted into the DOM and
> > +        attached. This prevents mutation events triggered by beforeload handlers from firing in the
> > +        midst of DOM insertion, which can lead to assertion failures and crashes.
> 
> Is the problem here really the mutation events, or merely the fact that the beforeload handler mutates the DOM during insertion of the link element?

You're right, "mutation event" is the wrong term to use.  I'll reword that before landing.

> 
> > +void HTMLLinkElement::processCallback(Node* n)
> > +{
> > +    static_cast<HTMLLinkElement*>(n)->process();
> > +}
> 
> I would assert (or ASSERT_ARG) that n is a link element before casting it.

Will do.

> 
> > +	<link rel="stylesheet" href="">
> 
> href="" means that the base URL (the .html file in this case) will be loaded as the stylesheet in this case. Not a big deal, but perhaps href="data:text/css," is cleaner (if it doesn’t invalidate the test).

Ahh, I didn't realize that.  I'll change that and re-test before landing.

Thanks for the review!
Comment 5 Darin Adler 2010-06-18 07:33:19 PDT
Comment on attachment 59074 [details]
Patch

WebCore/html/HTMLLinkElement.cpp:234
 +  void HTMLLinkElement::processCallback(Node* n)
We typically use words for variable and argument names, not letters. So "node" would be preferred over "n".
Comment 6 Andy Estes 2010-06-18 12:03:55 PDT
http://trac.webkit.org/changeset/61424
Comment 7 Eric Seidel (no email) 2010-06-18 14:08:47 PDT
Test appears to fail on Tiger.
Comment 8 Andy Estes 2010-06-18 14:22:50 PDT
(In reply to comment #7)
> Test appears to fail on Tiger.

The only test I see failing on Tiger is platform/mac/accessibility/inherited-presentational-lists.html.
Comment 9 Eric Seidel (no email) 2010-09-30 12:23:02 PDT
@ap: this seems like another symptom of StyleSheet's bad node-ownership design.  If you fix that, we may be able to remove this code.