Bug 76199 - nullptr crash in XMLDocumentParser::doEnd()
Summary: nullptr crash in XMLDocumentParser::doEnd()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: BrowserCompat, InRadar
Depends on: 90571
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-12 12:06 PST by Berend-Jan Wever
Modified: 2022-08-02 00:14 PDT (History)
15 users (show)

See Also:


Attachments
Repro (199 bytes, application/xhtml+xml)
2012-01-12 12:06 PST, Berend-Jan Wever
no flags Details
Patch (10.75 KB, patch)
2012-07-03 00:20 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2012-07-03 09:23 PDT, Caio Marcelo de Oliveira Filho
abarth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (525.20 KB, application/zip)
2012-07-03 10:00 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2012-01-12 12:06:29 PST
Created attachment 122283 [details]
Repro

Chromium: http://code.google.com/p/chromium/issues/detail?id=110058

Repro:
<html xmlns='http://www.w3.org/1999/xhtml'>
<script>
<![CDATA[
  document.addEventListener("DOMSubtreeModified",function(){
    try{window.stop()}catch(e){};
  },false);
]]>
</script>
<script
Comment 1 Caio Marcelo de Oliveira Filho 2012-07-03 00:20:21 PDT
Created attachment 150550 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-07-03 00:54:28 PDT
I thought we had fancy batched mutation stuff to fix this sort of thing?  Normally the parser doesn't want to fire mutation events anyway.  I don't think this is the fix you want.
Comment 3 Caio Marcelo de Oliveira Filho 2012-07-03 07:40:54 PDT
(In reply to comment #2)
> I thought we had fancy batched mutation stuff to fix this sort of thing?  Normally the parser doesn't want to fire mutation events anyway.  I don't think this is the fix you want.

Thanks for commenting, Eric.

I ran some tests: Firefox and Opera do not trigger the mutation event, changing the extension of the test to .html (use our HTML parser) also do not trigger it. Maybe a better fix here is to avoid this mutation event to be sent.
Comment 4 Caio Marcelo de Oliveira Filho 2012-07-03 09:23:13 PDT
Created attachment 150626 [details]
Patch
Comment 5 Adam Barth 2012-07-03 09:51:18 PDT
Comment on attachment 150626 [details]
Patch

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

Much better!

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:172
> +    const String bufferedText = toString(m_bufferedText.data(), m_bufferedText.size());

The const here isn't needed.
Comment 6 WebKit Review Bot 2012-07-03 10:00:05 PDT
Comment on attachment 150626 [details]
Patch

Attachment 150626 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13125548

New failing tests:
svg/foreignObject/text-tref-02-b.svg
svg/W3C-SVG-1.1-SE/text-tref-03-b.svg
Comment 7 WebKit Review Bot 2012-07-03 10:00:12 PDT
Created attachment 150634 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 8 Caio Marcelo de Oliveira Filho 2012-07-03 11:28:07 PDT
(In reply to comment #6)
> (From update of attachment 150626 [details])
> Attachment 150626 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13125548
> 
> New failing tests:
> svg/foreignObject/text-tref-02-b.svg
> svg/W3C-SVG-1.1-SE/text-tref-03-b.svg

Problem here is that SVGTRefElement was relying on the DOM mutation events sent by its target (another node), to update its own content, even during parsing.

Talking in IRC with abarth and maciej, SVGTRefElement shouldn't be doing this. I'm investigating other ways to handle TRef, so that we can land this patch without regressing it.
Comment 9 Eric Seidel (no email) 2012-07-03 11:38:33 PDT
No SVG definitely should not be doing this. :)
Comment 10 Nikolas Zimmermann 2012-07-05 04:25:03 PDT
(In reply to comment #8)
> Problem here is that SVGTRefElement was relying on the DOM mutation events sent by its target (another node), to update its own content, even during parsing.
> 

> Talking in IRC with abarth and maciej, SVGTRefElement shouldn't be doing this. I'm investigating other ways to handle TRef, so that we can land this patch without regressing it.

Good luck :-) Removing the need to rely on those events requires substantial work on <tref>. <tref> may reference _any_ node, to extract its textContent (even non-SVG nodes).

What we need is a mechanism to let <tref> know about textContent changes of arbitrary Nodes. If we have that, we can stop using mutation events for that in <tref>.

(Note: yes, it's crazy that <tref> allows references to all kind of Nodes, even non-SVG nodes, but that's how it's spec'd...)
Comment 11 Adam Barth 2012-07-05 07:05:01 PDT
How does <tref> work for SVG-in-HTML?  The HTML parser (correctly) doesn't generate mutation events.
Comment 12 Ryosuke Niwa 2012-07-05 11:46:41 PDT
(In reply to comment #10)
> What we need is a mechanism to let <tref> know about textContent changes of arbitrary Nodes. If we have that, we can stop using mutation events for that in <tref>.

arbitrary node in the subtree? or arbitrary node anywhere?
Comment 13 Caio Marcelo de Oliveira Filho 2012-07-05 12:11:02 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > What we need is a mechanism to let <tref> know about textContent changes of arbitrary Nodes. If we have that, we can stop using mutation events for that in <tref>.
> 
> arbitrary node in the subtree? or arbitrary node anywhere?

Apparently anywhere. The spec recommends authors to put nodes that will be referenced by <tref>s inside <defs> nodes. We could support that more easily I think, putting all the book keeping on <defs>. But we would leave other documents out. I'm not very familiar with SVG yet to make this call.
Comment 14 Ahmad Saleem 2022-08-01 12:48:49 PDT
I am still able to reproduce this bug in Safari 15.6 on macOS 12.5 using attached "Repro" test case and it crashes "tab" without any error etc. All other browsers are able to handle this reproduction test case and show error like following:

*** Chrome Canary 106 ***

This page contains the following errors:
error on line 9 at column 8: Couldn't find end of Start Tag script
Below is a rendering of the page up to the first error.

*** Firefox Nightly 105 ***

XML Parsing Error: unclosed token
Location: https://bug-76199-attachments.webkit.org/attachment.cgi?id=122283
Line Number 9, Column 1:
<script
^

_________

I think since it crashes tab, it would be good to look into.

Following commit fixed it in Chrome / Blink in 2015.

https://src.chromium.org/viewvc/blink?view=revision&revision=200026
Comment 15 Radar WebKit Bug Importer 2022-08-01 13:00:17 PDT
<rdar://problem/97931385>
Comment 17 Ryosuke Niwa 2022-08-01 21:45:52 PDT
The repro still reproduces a crash in WebKit.
Comment 18 Ryosuke Niwa 2022-08-01 22:11:40 PDT
Pull request: https://github.com/WebKit/WebKit/pull/2941
Comment 19 EWS 2022-08-02 00:14:43 PDT
Committed 253025@main (09c22643353b): <https://commits.webkit.org/253025@main>

Reviewed commits have been landed. Closing PR #2941 and removing active labels.