Last Comment Bug 672438 - Crash [@ nsINode::CompareDocPosition(nsINode*) ] with too many attr children
: Crash [@ nsINode::CompareDocPosition(nsINode*) ] with too many attr children
[inbound][sg:dos] null-pointer crash
: crash, reproducible
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
-- critical (vote)
: mozilla8
Assigned To: Mats Palmgren (:mats)
: Andrew Overholt [:overholt]
Depends on:
Blocks: 532972
  Show dependency treegraph
Reported: 2011-07-18 21:27 PDT by Bob Clary [:bc:]
Modified: 2011-07-21 06:13 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

aurora stack (3.54 KB, text/plain)
2011-07-18 21:27 PDT, Bob Clary [:bc:]
no flags Details
Like so? (2.18 KB, patch)
2011-07-20 00:51 PDT, Mats Palmgren (:mats)
hsivonen: review+
Details | Diff | Splinter Review

Description User image Bob Clary [:bc:] 2011-07-18 21:27:11 PDT
Created attachment 546706 [details]
aurora stack

2. Crash Aurora/7, Nightly/8 on Mac.

rapidly issues oodles of warnings then crash with Aurora. Nightly takes a bit longer. Haven't checked Beta/6.

aurora bp-8698ca1b-054a-41c5-8d01-4f6572110718

WARNING: NS_ENSURE_TRUE(childCount < ATTRCHILD_ARRAY_MAX_CHILD_COUNT) failed: file /work/mozilla/builds/aurora/mozilla/content/base/src/nsAttrAndChildArray.cpp, line 170
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /work/mozilla/builds/aurora/mozilla/content/base/src/nsGenericElement.cpp, line 3473

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x054254eb in nsINode::CompareDocPosition (this=0x0, aOtherNode=0x26151260) at /work/mozilla/builds/aurora/mozilla/content/base/src/nsGenericElement.cpp:774
774	  if (node2->IsNodeOfType(nsINode::eATTRIBUTE)) {

fwiw, this is another of these sites with broken php scripts that eat memory and crash the browser.

Automation did see a different stack with aurora:
NS_DebugBreak_P | nsCOMPtr<nsIParser>::operator-> | nsScriptElement::MaybeProcessScript nsHTMLScriptElement::BindToTree nsINode::doInsertChildAt nsGenericElement::InsertChildAt nsINode::AppendChildTo  

The original Socorro signature was mozalloc_abort(char const* const) | mozalloc_handle_oom() | nsTArray_base<nsTArrayDefaultAllocator>::EnsureCapacity(unsigned int, unsigned int) | nsTArray<nsHtml5TreeOperation, nsTArrayDefaultAllocator>::MoveElementsFrom<nsHtml5TreeOperation, nsTArrayD...
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 06:43:29 PDT
Jonas, can you take a look?
Comment 2 User image Mats Palmgren (:mats) 2011-07-20 00:48:39 PDT
PostPendingAppendNotification is called before AppendChildTo:
which results in a new nsHtml5PendingNotification object:
with mChildCount = aParent->GetChildCount().

The bug occurs when the child count has reached the maximum value,
then AppendChildTo() fails and mChildCount has a value that isn't
a valid index so the mParent->GetChildAt(mChildCount) at line 60
returns null leading to the null-pointer crash.
Comment 3 User image Mats Palmgren (:mats) 2011-07-20 00:51:40 PDT
Created attachment 547002 [details] [diff] [review]
Like so?

Notifying only if AppendChildTo succeeds seems like the way to go.
Comment 4 User image Henri Sivonen (:hsivonen) 2011-07-20 07:24:00 PDT
Comment on attachment 547002 [details] [diff] [review]
Like so?

Looks reasonable in order to fix the crash.

I wonder if we should try, as a more elaborate follow-up, to halt the parser on unload the document upon failures that leave the DOM in a state that doesn't correspond to what the parser saw.
Comment 5 User image Mats Palmgren (:mats) 2011-07-20 13:33:55 PDT
Filed bug 672928 to follow-up Henri's review comment.
Comment 6 User image Marco Bonardo [::mak] 2011-07-21 06:13:09 PDT

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