The default bug view has changed. See this FAQ.

[HTML5] Crash [@ nsHtml5PendingNotification::nsHtml5PendingNotification(nsIContent*)] on Forbes.com

RESOLVED FIXED

Status

()

Core
HTML: Parser
P1
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

({crash, topcrash})

Trunk
x86
All
crash, topcrash
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [#3 trunk (3.7a1) topcrash], crash signature, URL)

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
With speculative HTML5 parsing, http://www.forbes.com/ causes a text node to be appended to a null parent, which causes a pending append notification to be posted with null argument.
(Assignee)

Comment 1

8 years ago
Trying to debug this with assertions, I see non-sensical stacks. Also, I managed to get a non-null bogus pointer at the crash site.

I suspect heap corruption. :-(
(Assignee)

Comment 2

8 years ago
Maybe the non-sensical stacks were due to the debugger breaking on the main thread when another thread sends an interrupt signal (when assertions are set to trap).

The value of the text node being supposedly appended to the 0x0 parent node is "\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklm"

Which suggests the tree op is completely corrupted. Moreover, I tried asserting that an append is never created with a null parent but that assertion never fires.

I also discovered that Valgrind doesn't work on Snow Leopard. :-( I'll try on Linux.
(Assignee)

Comment 3

8 years ago
It crashes in the same place on Linux, which (fortunately) suggests that this isn't badness caused by a double-free or something like that. Valgrind doesn't complain about anything prior to the crash.

Upon crash in Valgrind on Linux, the bad pointer is 0xa5a5a5a5 (0x0 on Mac without Valgrind). So far I've been unable to locate documentation saying what marker pattern 0xa5a5a5a5 is.

Anyway, it looks like the parent pointer is uninitialized but somehow the opcode is an append. It seems to me that whenever eTreeOpAppend is used in an initialization, it is used with the right number of operands. I'm now asserting the non-nullness of those operands, but those assertions don't fire.

So:
 * The same crash on at least two platforms.
 * Consistent crash.
 * The second pointer in the tree op is uninitialized.
 * The opcode is append. (Or at least something that acts like append in switch!)
 * The first pointer is interpreted (by gdb) to be a text node, but its memory looks bogus.
 * The tree op creation code in the tree builder looks correct.

What could touch the tree op memory behind the back of the tree builder?
(Assignee)

Comment 4

8 years ago
So 0xa5a5a5a5 is jemalloc uninitialized memory.
(Assignee)

Comment 5

8 years ago
What must be happening here is that each tree op is initialized alright but when the append runs, previous tree ops haven't assigned the right pointers to the nsIContent** handles. Specifically, the createElement operation for the parent hasn't run.

This is odd. I thought that whenever ops got trashed, the creation ops and subsequent ops depending on the creation ops would get trashed together.
(Assignee)

Comment 6

8 years ago
One queue of tree ops has a handle for a td element. A subsequent run of tree ops starts with creating a text node for "\n" and appending it to td element, but the td element was never created.

It doesn't help to clear the text buffer of the tree builder and to trash the op queue of the tree builder when a speculation fails.
(Assignee)

Comment 7

8 years ago
Created attachment 410761 [details] [diff] [review]
Be more careful about emptying the tree builder op queue

The actual fix is in the two first hunks of the diff. The rest is just added debug code that's nice to put in the tree, too, IMO.

So what happened here was that an element got created as a result of document.write(). The stack node for that element was properly cloned to the parser thread but I had forgotten to transfer the tree ops from the document.write() tree builder to the executor before going back to the off-the-main-thread parser.
Attachment #410761 - Flags: review?(bnewman)
(Assignee)

Comment 8

8 years ago
This really needs a regression test, too.
Flags: in-testsuite?
(Assignee)

Comment 9

8 years ago
Created attachment 411409 [details] [diff] [review]
Crashtest
Attachment #411409 - Flags: review?(bnewman)
Attachment #410761 - Flags: review?(bnewman) → review+
Attachment #411409 - Flags: review?(bnewman) → review+
(Assignee)

Updated

8 years ago
Duplicate of this bug: 529956
(Assignee)

Updated

8 years ago
Priority: -- → P1

Updated

7 years ago
Duplicate of this bug: 530279

Updated

7 years ago
OS: Mac OS X → All
Duplicate of this bug: 530279
Keywords: crash, topcrash
Whiteboard: [#3 trunk (3.7a1) topcrash]
Summary: [HTML5] Crash [@ nsHtml5PendingNotification::nsHtml5PendingNotification] on Forbes.com → [HTML5] Crash [@ nsHtml5PendingNotification::nsHtml5PendingNotification(nsIContent*)] on Forbes.com
washingtonpost.com is crashing the latest Minefield builds as well and I believe it's the same bug. Here's my crash reports:

http://crash-stats.mozilla.com/report/index/5a7738b3-f7b6-4602-8720-160cb2091129
http://crash-stats.mozilla.com/report/index/e0ef5b71-155e-4064-84b1-64e392091129

Comment 14

7 years ago
I've had this crash on three separate sites today and then I found this through the crash report. How is progress?
(Assignee)

Comment 15

7 years ago
(In reply to comment #14)
> I've had this crash on three separate sites today and then I found this through
> the crash report. How is progress?

The crash is understood and has a reviewed fix. However, the trunk is restricted to 1.9.2 blockers and this isn't a 1.9.2 blocker. Hence, I'm waiting for the tree to reopen. I'm sorry for the prolonged unfixedness.

For the time being, if you aren't specifically testing HTML5 parsing, it's probably best to set html5.enable to false until the patch for this bug lands. OTOH, if you are testing HTML5 parsing specifically, tryserver builds with the fix for this bug (and other HTML5 parsing fixes, including Web compat fixes to the parsing algorithm itself) are available from
https://build.mozilla.org/tryserver-builds/hsivonen@iki.fi-try-684b586fde4c/
a=beltzner for landing on mozilla-central, this is not part of the build as it's off by default at the moment so it won't interfere with mozilla-1.9.2 landings.
(Assignee)

Comment 17

7 years ago
Thanks for r & a. Pushed:
http://hg.mozilla.org/mozilla-central/rev/2f11a4721c94

(I didn't push the test yet, to avoid pushing anything that'd run on tinderboxen.)
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Duplicate of this bug: 530881
(Assignee)

Updated

7 years ago
Duplicate of this bug: 531574
(Assignee)

Comment 20

7 years ago
Test pushed:
http://hg.mozilla.org/mozilla-central/rev/0c6ce6f93f19
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsHtml5PendingNotification::nsHtml5PendingNotification(nsIContent*)]
You need to log in before you can comment on or make changes to this bug.