The default bug view has changed. See this FAQ.

Crash when document.close() called with document.write() (of the same doc) already on the call stack

RESOLVED FIXED in Firefox 10

Status

()

Core
HTML: Parser
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bc, Assigned: hsivonen)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla10
x86
All
crash, regression, reproducible, verified-beta
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox8 unaffected, firefox9 unaffected, firefox10 verified)

Details

(Whiteboard: [qa+][qa!:10], crash signature, URL)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
1. http://code.google.com/p/google-caja/
2. Crash Nightly Linux, Mac, Windows

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xfffffffc
0x05e688b0 in nsHtml5TreeBuilder::currentNode (this=0x25708e00) at /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5TreeBuilder.cpp:3739
3739	  return stack[currentPtr]->node;
(gdb) bt
#0  0x05e688b0 in nsHtml5TreeBuilder::currentNode (this=0x25708e00) at /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5TreeBuilder.cpp:3739
#1  0x05e6f3a0 in nsHtml5TreeBuilder::flushCharacters (this=0x25708e00) at /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5TreeBuilder.cpp:3778
#2  0x05e6f441 in nsHtml5TreeBuilder::Flush (this=0x25708e00, aDiscretionary=false) at nsHtml5TreeBuilderCppSupplement.h:620
#3  0x05e332e4 in nsHtml5Parser::Parse (this=0x25708f00, aSourceBuffer=@0xbfffb4a4, aKey=0x0, aContentType=@0xbfffb410, aLastCall=false, aMode=eDTDMode_autodetect) at /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5Parser.cpp:411
#4  0x05b5464b in nsHTMLDocument::WriteCommon (this=0xe82e00, cx=0x1ebd80, aText=@0xbfffb4a4, aNewlineTerminate=false) at /work/mozilla/builds/nightly/mozilla/content/html/document/src/nsHTMLDocument.cpp:1954
#

(gdb) p currentPtr
$1 = -1

Comment 1

6 years ago
https://crash-stats.mozilla.com/report/index/bp-920ddd2a-fa73-4493-9f20-58bbe2111023
(Assignee)

Comment 2

6 years ago
This is related to document.close().

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6d5fd5a30c71&tochange=3b58a9df4c8c
(Assignee)

Updated

6 years ago
Depends on: 687744
(Assignee)

Comment 3

6 years ago
https://hg.mozilla.org/mozilla-central/rev/daef044609ce is to blame.
(Assignee)

Comment 4

6 years ago
Created attachment 569328 [details] [diff] [review]
Deal with termination and document.close() better when document.write() is on the call stack

I was unable to figure out why https://hg.mozilla.org/mozilla-central/rev/daef044609ce provoked this crash.

Fortunately, I was able to figure out how to fix the crash.

Next, I'll see if I can provoke the crash intentionally with a crashtest.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
Created attachment 569334 [details] [diff] [review]
Crashtest
(Assignee)

Updated

6 years ago
Crash Signature: [@ nsHtml5TreeBuilder::flushCharacters() ] [@ nsHtml5TreeBuilder::currentNode ] → [@ nsHtml5TreeBuilder::flushCharacters() ] [@ nsHtml5TreeBuilder::currentNode ] [@ nsHtml5Tokenizer::stateLoop(int, wchar_t, int, wchar_t*, bool, int, int)] [@ nsHtml5Tokenizer::stateLoop ]
Summary: Crash [@ nsHtml5TreeBuilder::flushCharacters() ] → Crash when document.close() called with document.write() (of the same doc) already on the call stack
(Assignee)

Updated

6 years ago
Duplicate of this bug: 695041
(Assignee)

Updated

6 years ago
Keywords: regressionwindow-wanted
(Assignee)

Updated

6 years ago
Attachment #569328 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Attachment #569334 - Flags: review?(Olli.Pettay)
Attachment #569334 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 569328 [details] [diff] [review]
Deal with termination and document.close() better when document.write() is on the call stack

Waiting for explanation for the RunnableMethod
Attachment #569328 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 8

6 years ago
Created attachment 569611 [details] [diff] [review]
Deal with termination and document.close() better when document.write() is on the call stack, without runnable method

Things seem to work fine without the runnable method, so I removed it.
Attachment #569328 - Attachment is obsolete: true
Attachment #569611 - Flags: review?(Olli.Pettay)
Comment on attachment 569611 [details] [diff] [review]
Deal with termination and document.close() better when document.write() is on the call stack, without runnable method

So this looks good, but I'd like to know what caused the regression.
Attachment #569611 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 570476 [details] [diff] [review]
Part 2: Establish the document.write() insertion location before re-entry can occur
Attachment #570476 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 11

6 years ago
Created attachment 570477 [details] [diff] [review]
Reftest for part 2
Attachment #570477 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 12

6 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> So this looks good, but I'd like to know what caused the regression.

Before this regressed, the document.write() buffer got inserted into the stream first, so it was already in buffer list so that document.close() was able to exhaust it unexpectedly soon. (I've spent so much time on this bug, that I didn't verify this to the last detail in a debugger, but I believe this is the explanation.)

The part 2 patch moves enough of the buffer list management before the flush that the insertion points become right again in cases that involve both re-entry and blocking (see the reftests).
Attachment #570477 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 570476 [details] [diff] [review]
Part 2: Establish the document.write() insertion location before re-entry can occur


>+  nsHtml5OwningUTF16Buffer* prevSearchBuf = nsnull;
>+  nsHtml5OwningUTF16Buffer* firstLevelMarker = nsnull;
Is it guaranteed that these objects stay alive while flushing, or should you perhaps
use nsRefPtr?
Or could all the processing happen before flush?
Attachment #570476 - Flags: review?(Olli.Pettay) → review+
s/processing/buffer handling/
(Assignee)

Comment 15

6 years ago
(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 570476 [details] [diff] [review] [diff] [details] [review]
> Part 2: Establish the document.write() insertion location before re-entry
> can occur
> 
> 
> >+  nsHtml5OwningUTF16Buffer* prevSearchBuf = nsnull;
> >+  nsHtml5OwningUTF16Buffer* firstLevelMarker = nsnull;
> Is it guaranteed that these objects stay alive while flushing, or should you
> perhaps
> use nsRefPtr?

These pointers always point to buffers that are in the linked list of buffers, so they are kept alive by the linked list staying alive. document.write() never removes anything from the list.

Thus, letting these be non-owning pointers is OK. (bz has seen the linked list search show up on profiles in a pathological case, so I figured I'd avoid making the search slower by AddRefing and Releasing a lot when it's not necessary.)

> Or could all the processing happen before flush?
> s/processing/buffer handling/

All buffer handling used to happen before the flush. Moving it all back to before the flush would undo some aspects of bug 687744 I thought we wanted to keep. (Specifically, in cases where aKey is not null and the entire buffer is consumed, with this patch here the buffer queue isn't bloated by a useless empty buffer but it was bloated by a useless empty buffer before bug 687744 was fixed. bz had seen a pathological case where there ended up being a lot of those useless buffer objects in the linked list.)
(Assignee)

Comment 16

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0847ed11f271
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f67b8cc3a8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1766aec3d7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4ddddf142d

Thanks for the reviews.
Flags: in-testsuite+
Target Milestone: --- → mozilla10
(In reply to Henri Sivonen (:hsivonen) from comment #15)
> so I figured I'd
> avoid making the search slower by AddRefing and Releasing a lot when it's
> not necessary.)

Addref/Release are non-virtual in this case, and non-thread-safe, so they should be really fast.
(Assignee)

Comment 18

6 years ago
And backed out part 2 due to jsreftest orange (which is strange, because this looked ok on try):
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a12d693b5b
Part 1:
https://hg.mozilla.org/mozilla-central/rev/0847ed11f271
https://hg.mozilla.org/mozilla-central/rev/9f67b8cc3a8f

Leaving open until part 2 relands.
(Assignee)

Comment 20

6 years ago
Created attachment 570637 [details] [diff] [review]
Part 3: Fix part 2

This patch:
 * Adds comments
 * Deals with the case where mFirstBuffer is already the key holder in the pre-flush search.
 * Removes reftest ~ backup files.
Attachment #570637 - Flags: review?(Olli.Pettay)
Comment on attachment 570637 [details] [diff] [review]
Part 3: Fix part 2

>   if (aKey) {
>     if (mFirstBuffer == mLastBuffer) {
>       nsHtml5OwningUTF16Buffer* keyHolder = new nsHtml5OwningUTF16Buffer(aKey);
>       keyHolder->next = mLastBuffer;
>       mFirstBuffer = keyHolder;
>-    } else {
>+    } else if (mFirstBuffer->key != aKey) {
>       prevSearchBuf = mFirstBuffer;
>       for (;;) {
>         if (prevSearchBuf->next == mLastBuffer) {
>           // key was not found
>           nsHtml5OwningUTF16Buffer* keyHolder =
>             new nsHtml5OwningUTF16Buffer(aKey);
>           keyHolder->next = mFirstBuffer;
>           mFirstBuffer = keyHolder;
>@@ -357,17 +361,18 @@ nsHtml5Parser::Parse(const nsAString& aS
>           break;
>         }
>         if (prevSearchBuf->next->key == aKey) {
>           // found a key holder
>           break;
>         }
>         prevSearchBuf = prevSearchBuf->next;
>       }
>-    }
>+    } // else mFirstBuffer is the keyholder
>+

This key handling is obviously too error prone. But I'm not sure we have any good helper classes to handle
linked lists in this kind of case.
Attachment #570637 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 22

6 years ago
Relanded.
https://hg.mozilla.org/integration/mozilla-inbound/rev/887e93b8c105
https://hg.mozilla.org/integration/mozilla-inbound/rev/623b4278134f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dbe9cf0a485

(In reply to Olli Pettay [:smaug] from comment #17)
> (In reply to Henri Sivonen (:hsivonen) from comment #15)
> > so I figured I'd
> > avoid making the search slower by AddRefing and Releasing a lot when it's
> > not necessary.)
> 
> Addref/Release are non-virtual in this case, and non-thread-safe, so they
> should be really fast.

Used nsRefPtr per IRC discussion.

(In reply to Olli Pettay [:smaug] from comment #21)

> This key handling is obviously too error prone. But I'm not sure we have any
> good helper classes to handle
> linked lists in this kind of case.

The code here is specialized enough that chances are that a helper class that did the right thing and didn't traverse the list more than necessary would be as much a one-off piece of code as the code here. :-(
Parts 2 and 3:
https://hg.mozilla.org/mozilla-central/rev/887e93b8c105
https://hg.mozilla.org/mozilla-central/rev/623b4278134f
https://hg.mozilla.org/mozilla-central/rev/1dbe9cf0a485
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox10: affected → fixed
Resolution: --- → FIXED

Comment 24

6 years ago
I looked into why my DOM fuzzer missed this bug.  I didn't really figure out why, but I made some changes that should help. (fuzzing rev 76a5ebb74e8e)
Whiteboard: [qa+]
No crashes. This is verified fixed on Firefox 10 Beta3:
Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0
status-firefox10: fixed → verified
Keywords: verified-beta
Whiteboard: [qa+] → [qa+][qa!:10]
You need to log in before you can comment on or make changes to this bug.