Last Comment Bug 696651 - Crash when document.close() called with document.write() (of the same doc) already on the call stack
: Crash when document.close() called with document.write() (of the same doc) al...
Status: RESOLVED FIXED
[qa+][qa!:10]
: crash, regression, reproducible, verified-beta
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: mozilla10
Assigned To: Henri Sivonen (:hsivonen)
:
: Andrew Overholt [:overholt]
Mentors:
http://code.google.com/p/google-caja/
: 695041 (view as bug list)
Depends on: 687744
Blocks: 532972
  Show dependency treegraph
 
Reported: 2011-10-23 10:07 PDT by Bob Clary [:bc:]
Modified: 2012-01-10 06:23 PST (History)
9 users (show)
hsivonen: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
verified


Attachments
Deal with termination and document.close() better when document.write() is on the call stack (3.36 KB, patch)
2011-10-25 04:42 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Crashtest (1.30 KB, patch)
2011-10-25 05:28 PDT, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review
Deal with termination and document.close() better when document.write() is on the call stack, without runnable method (3.25 KB, patch)
2011-10-25 23:00 PDT, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review
Part 2: Establish the document.write() insertion location before re-entry can occur (11.98 KB, patch)
2011-10-29 07:37 PDT, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review
Reftest for part 2 (3.74 KB, patch)
2011-10-29 07:38 PDT, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review
Part 3: Fix part 2 (3.55 KB, patch)
2011-10-31 04:00 PDT, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review

Description Bob Clary [:bc:] 2011-10-23 10:07:18 PDT
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 2 Henri Sivonen (:hsivonen) 2011-10-24 08:27:33 PDT
This is related to document.close().

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6d5fd5a30c71&tochange=3b58a9df4c8c
Comment 3 Henri Sivonen (:hsivonen) 2011-10-24 23:48:00 PDT
https://hg.mozilla.org/mozilla-central/rev/daef044609ce is to blame.
Comment 4 Henri Sivonen (:hsivonen) 2011-10-25 04:42:26 PDT
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.
Comment 5 Henri Sivonen (:hsivonen) 2011-10-25 05:28:47 PDT
Created attachment 569334 [details] [diff] [review]
Crashtest
Comment 6 Henri Sivonen (:hsivonen) 2011-10-25 06:53:23 PDT
*** Bug 695041 has been marked as a duplicate of this bug. ***
Comment 7 Olli Pettay [:smaug] 2011-10-25 10:04:12 PDT
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
Comment 8 Henri Sivonen (:hsivonen) 2011-10-25 23:00:22 PDT
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.
Comment 9 Olli Pettay [:smaug] 2011-10-28 15:11:41 PDT
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.
Comment 10 Henri Sivonen (:hsivonen) 2011-10-29 07:37:48 PDT
Created attachment 570476 [details] [diff] [review]
Part 2: Establish the document.write() insertion location before re-entry can occur
Comment 11 Henri Sivonen (:hsivonen) 2011-10-29 07:38:22 PDT
Created attachment 570477 [details] [diff] [review]
Reftest for part 2
Comment 12 Henri Sivonen (:hsivonen) 2011-10-29 07:42:05 PDT
(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).
Comment 13 Olli Pettay [:smaug] 2011-10-29 08:33:41 PDT
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?
Comment 14 Olli Pettay [:smaug] 2011-10-29 08:34:05 PDT
s/processing/buffer handling/
Comment 15 Henri Sivonen (:hsivonen) 2011-10-29 13:09:34 PDT
(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.)
Comment 17 Olli Pettay [:smaug] 2011-10-29 13:54:23 PDT
(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.
Comment 18 Henri Sivonen (:hsivonen) 2011-10-29 14:13:43 PDT
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
Comment 19 Ed Morley [:emorley] 2011-10-30 10:35:40 PDT
Part 1:
https://hg.mozilla.org/mozilla-central/rev/0847ed11f271
https://hg.mozilla.org/mozilla-central/rev/9f67b8cc3a8f

Leaving open until part 2 relands.
Comment 20 Henri Sivonen (:hsivonen) 2011-10-31 04:00:36 PDT
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.
Comment 21 Olli Pettay [:smaug] 2011-10-31 06:41:02 PDT
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.
Comment 22 Henri Sivonen (:hsivonen) 2011-10-31 07:32:46 PDT
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. :-(
Comment 24 Jesse Ruderman 2011-11-07 19:42:02 PST
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)
Comment 25 Paul Silaghi, QA [:pauly] 2012-01-10 06:23:14 PST
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

Note You need to log in before you can comment on or make changes to this bug.