Bug 149485 – [WK1] Null dereference loading Blink layout test fast/parser/xhtml-dom-character-data-modified-crash.html

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jon Honeycutt]

Created attachment 261779 [details]
crashing test

Null dereference loading Blink layout test parser/xhtml-dom-character-data-modified-crash.html.

Test added in https://codereview.chromium.org/1267283002.


Stack trace:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000

Application Specific Information:
CRASHING TEST: temp-tests/fast/parser/xhtml-dom-character-data-modified-crash.html
This process is running with libgmalloc.dylib (GuardMalloc) which may have forced the crash due to a memory access error.
 

Global Trace Buffer (reverse chronological seconds):
48.335848    CFNetwork                 	0x00007fff88d445cd Creating default cookie storage with process/bundle identifier
48.335848    CFNetwork                 	0x00007fff88d44565 Faulting in CFHTTPCookieStorage singleton
48.335855    CFNetwork                 	0x00007fff88ba7b49 Faulting in NSHTTPCookieStorage singleton

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000107361a52 WebCore::XMLDocumentParser::endElementNs() + 130 (XMLDocumentParserLibxml2.cpp:887)
1   libxml2.2.dylib               	0x00007fff8c5a1d16 xmlParseEndTag2 + 917
2   libxml2.2.dylib               	0x00007fff8c5a42fe xmlParseTryOrFinish + 3216
3   libxml2.2.dylib               	0x00007fff8c5a3479 xmlParseChunk + 905
4   com.apple.WebCore             	0x000000010736064b WebCore::XMLDocumentParser::doWrite(WTF::String const&) + 475 (Vector.h:634)
5   com.apple.WebCore             	0x000000010735e590 WebCore::XMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) + 208 (StdLibExtras.h:366)
6   com.apple.WebCore             	0x0000000106586565 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter&, char const*, unsigned long) + 117 (StdLibExtras.h:366)
7   com.apple.WebCore             	0x00000001065cf051 WebCore::DocumentLoader::commitData(char const*, unsigned long) + 657 (DocumentLoader.cpp:857)
8   com.apple.WebKitLegacy        	0x0000000108a3ea82 -[WebHTMLRepresentation receivedData:withDataSource:] + 98 (WebHTMLRepresentation.mm:198)
9   com.apple.WebKitLegacy        	0x0000000108a0d7a0 -[WebDataSource(WebInternal) _receivedData:] + 64 (WebDataSource.mm:242)
10  com.apple.WebKitLegacy        	0x0000000108a2af9d WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) + 109 (WebFrameLoaderClient.mm:1014)
11  com.apple.WebCore             	0x00000001065d0b01 WebCore::DocumentLoader::commitLoad(char const*, int) + 145 (DocumentLoader.h:225)
12  com.apple.WebCore             	0x00000001063fb250 WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) + 160 (CachedResourceClientWalker.h:51)
13  com.apple.WebCore             	0x00000001063fb121 WebCore::CachedRawResource::addDataBuffer(WebCore::SharedBuffer&) + 145 (CachedRawResource.cpp:70)
14  com.apple.WebCore             	0x0000000107175f8a WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::PassRefPtr<WebCore::SharedBuffer>, long long, WebCore::DataPayloadType) + 218 (SubresourceLoader.cpp:292)
15  com.apple.WebCore             	0x000000010717603c WebCore::SubresourceLoader::didReceiveBuffer(WTF::PassRefPtr<WebCore::SharedBuffer>, long long, WebCore::DataPayloadType) + 44 (StdLibExtras.h:366)
16  com.apple.WebCore             	0x000000010700ea7c WebCore::ResourceLoader::didReceiveBuffer(WebCore::ResourceHandle*, WTF::PassRefPtr<WebCore::SharedBuffer>, int) + 44 (StdLibExtras.h:366)
17  com.apple.WebCore             	0x00000001072f2f6a -[WebCoreResourceHandleAsDelegate connection:didReceiveDataArray:] + 106 (StdLibExtras.h:366)
18  com.apple.CFNetwork           	0x00007fff88c5c622 __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke + 69
19  com.apple.CFNetwork           	0x00007fff88c5c49e -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] + 233
20  com.apple.CFNetwork           	0x00007fff88c5c3a3 -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] + 48
21  com.apple.CFNetwork           	0x00007fff88da98fc _NSURLConnectionDidReceiveDataArray(_CFURLConnection*, __CFArray const*, void const*) + 82
22  com.apple.CFNetwork           	0x00007fff88c5cc61 ___ZN27URLConnectionClient_Classic29_delegate_didReceiveDataArrayEv_block_invoke + 145
23  com.apple.CFNetwork           	0x00007fff88d3ad5f ___ZN27URLConnectionClient_Classic18_withDelegateAsyncEPKcU13block_pointerFvP16_CFURLConnectionPK33CFURLConnectionClientCurrent_VMaxE_block_invoke_2 + 100
24  libdispatch.dylib             	0x00007fff96096331 _dispatch_client_callout + 8
25  libdispatch.dylib             	0x00007fff960ac9ad _dispatch_block_invoke + 474
26  com.apple.CFNetwork           	0x00007fff88bb67f4 RunloopBlockContext::_invoke_block(void const*, void*) + 24
27  com.apple.CoreFoundation      	0x00007fff949aeff4 CFArrayApplyFunction + 68
28  com.apple.CFNetwork           	0x00007fff88bb66ed RunloopBlockContext::perform() + 137
29  com.apple.CFNetwork           	0x00007fff88bb64c4 MultiplexerSource::perform() + 282
30  com.apple.CFNetwork           	0x00007fff88bb62e6 MultiplexerSource::_perform(void*) + 72
31  com.apple.CoreFoundation      	0x00007fff949e2c01 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
32  com.apple.CoreFoundation      	0x00007fff949d4b1c __CFRunLoopDoSources0 + 556
33  com.apple.CoreFoundation      	0x00007fff949d403f __CFRunLoopRun + 927
34  com.apple.CoreFoundation      	0x00007fff949d3a38 CFRunLoopRunSpecific + 296
35  DumpRenderTree                	0x0000000104bf0eb3 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2146 (DumpRenderTree.mm:2031)
36  DumpRenderTree                	0x0000000104bf041a dumpRenderTree(int, char const**) + 2928 (DumpRenderTree.mm:1288)
37  DumpRenderTree                	0x0000000104bf1a2a DumpRenderTreeMain(int, char const**) + 1471 (DumpRenderTree.mm:1424)
38  libdyld.dylib                 	0x00007fff93aa15ad start + 1

[Radar WebKit Bug Importer]

<rdar://problem/22811489>

[Jon Honeycutt]

Created attachment 261780 [details]
subresource

[Jiewen Tan]

Created attachment 262214 [details]
Patch

[Darin Adler]

Comment on attachment 262214 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:165
>      Vector<xmlChar> empty;
>      m_bufferedText.swap(empty);

Not sure I understand the value of this idiom over simpler ways of writing it, like this:

    m_bufferedText = { };

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:170
> +    // Mutation event handlers executed by appendData() might detach this parser.
> +    return !isStopped();

This comment is super-mysterious. We make this statement, but don’t establish its relevance, nor explain what the return value of the function means.

We should start by adding a comment somewhere that explains the meaning of the return value; I think it returns false if we should stop processing data. I think that’s a bit strange. In fact, why don’t the callers just check isStopped themselves? Why do we need a return value?

Then the comment here would say something like this:

    // We need to check again whether the parser is stopped, since mutation
    // event handlers executed by appendData might have detached this parser.

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:201
> +        // Do not bail out if in a stopped state, but notify document that
> +        // parsing has finished.

This comment is confusing. I think it’s referring to not looking at the return value of updateLeafTextNode, but the phrase “notify document that parsing has finished” seems to refer to code way further down in the function. I suggest just omitting the comment unless there’s something useful to say. Maybe:

    // No need to bail out if the parser is in a stopped state because ...

where you fill in the reason. But I think just omitting the comment is better than what you have here.

[Jiewen Tan]

Created attachment 262279 [details]
Patch

[Alexey Proskuryakov]

Are we still expected to dispatch synchronous mutation events here? I thought that this was going away.

See bug 81920, bug 85161, bug 68729, bug 64532.

[Jiewen Tan]

Created attachment 262336 [details]
Patch

[Chris Dumez]

(In reply to comment #6)
> Are we still expected to dispatch synchronous mutation events here? I
> thought that this was going away.
> 
> See bug 81920, bug 85161, bug 68729, bug 64532.

To me it would seem a bit early to drop DOMCharacterDataModified. Even though, it is being phased out, Chrome still supports it for example (although they have a UseCounter on it).

[Jiewen Tan]

(In reply to comment #6)
> Are we still expected to dispatch synchronous mutation events here? I
> thought that this was going away.
> 
> See bug 81920, bug 85161, bug 68729, bug 64532.

That I don't know. What's your opinion? I will CC Jon as well.

[Alexey Proskuryakov]

I expect that someone else CC'ed on the bug will know more about these discussions than myself.

[Darin Adler]

Comment on attachment 262336 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:156
> -void XMLDocumentParser::exitText()
> +bool XMLDocumentParser::updateLeafTextNode()

Some day we should return and fix this confusing idiom. It’s not at all clear what the return value means, nor does it seem that callers need this function to do the checks for them. The caller could just check isStopped().

[WebKit Commit Bot]

Comment on attachment 262336 [details]
Patch

Clearing flags on attachment: 262336

Committed r190760: <http://trac.webkit.org/changeset/190760>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.