Last Comment Bug 525276 - crashes [@ nsDocument::RegisterNamedItems(nsIContent*)]
: crashes [@ nsDocument::RegisterNamedItems(nsIContent*)]
Status: VERIFIED FIXED
[sg:dos null-deref][fixed by 468562]
: crash, regression, topcrash, verified1.9.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- critical with 1 vote (vote)
: mozilla1.9.1
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 468562
Blocks: 502168
  Show dependency treegraph
 
Reported: 2009-10-29 11:48 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
samuel.sidler+old: wanted1.9.0.x?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
.5+
.5-fixed


Attachments
wip (2.77 KB, patch)
2009-10-30 05:02 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-10-29 11:48:49 PDT
Crashes at nsDocument::RegisterNamedItems(nsIContent*) are the #20 topcrash for Firefox 3.5.4 so far.

Two of the 7 user comments are in Turkish, and a third references a turkish site:

whenever i click to link http://www.hurriyet.com.tr/spor/, it crashes!!!

The other comment with a URL is:
Was trying to do a "advanced search" on http://itu.org when it crashed. Had something like 20 tabs active


Full list of crashes at:
http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.5.4&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsDocument%3A%3ARegisterNamedItems%28nsIContent*%29



This is showing up on both Windows and Mac, and the top of the stack generally seems to be:

0  	xul.dll  	nsDocument::RegisterNamedItems  	 content/base/src/nsDocument.cpp:2364
1 	xul.dll 	nsDocument::ContentInserted 	content/base/src/nsDocument.cpp:2401
2 	xul.dll 	nsNodeUtils::ContentInserted 	content/base/src/nsNodeUtils.cpp:144
3 	xul.dll 	HTMLContentSink::NotifyInsert 	content/html/document/src/nsHTMLContentSink.cpp:3039
Comment 1 Samuel Sidler (old account; do not CC) 2009-10-29 15:23:46 PDT
We need an owner here. We're already talking about a short cycle for the new #1 topcrash. It'd be good to fix this as well.
Comment 2 Samuel Sidler (old account; do not CC) 2009-10-29 17:24:53 PDT
Boris: Johnny suggested this might be related to changes you made during the 3.5.4 cycle. I think he was thinking of bug 489925, which touched that function.

I can work up some URLs if you think that'd help. Let me know.
Comment 3 Boris Zbarsky [:bz] 2009-10-29 22:46:55 PDT
> I think he was thinking of bug 489925

The patch landed in that bug was backed out in bug 522030.
Comment 4 Boris Zbarsky [:bz] 2009-10-29 22:53:32 PDT
Typical stack:

0 	xul.dll 	nsDocument::RegisterNamedItems 	content/base/src/nsDocument.cpp:2364
1 	xul.dll 	nsDocument::ContentInserted 	content/base/src/nsDocument.cpp:2401
2 	xul.dll 	nsNodeUtils::ContentInserted 	content/base/src/nsNodeUtils.cpp:144
3 	xul.dll 	HTMLContentSink::NotifyInsert 	content/html/document/src/nsHTMLContentSink.cpp:3039
4 	xul.dll 	xul.dll@0x32bbb6 	
5 	xul.dll 	HTMLContentSink::FlushTags 	content/html/document/src/nsHTMLContentSink.cpp:3237

6 	xul.dll 	nsContentSink::BeginUpdate 	content/base/src/nsContentSink.cpp:1609
7 	xul.dll 	nsDocument::BeginUpdate 	content/base/src/nsDocument.cpp:3752
8 	xul.dll 	CSSLoaderImpl::InsertSheetInDoc 	layout/style/nsCSSLoader.cpp:1235
9 	xul.dll 	CSSLoaderImpl::LoadStyleLink 	layout/style/nsCSSLoader.cpp:1826
10 	xul.dll 	nsStyleLinkElement::DoUpdateStyleSheet 	content/base/src/nsStyleLinkElement.cpp:313
11 	xul.dll 	nsGenericElement::InsertChildAt 	content/base/src/nsGenericElement.cpp:3203
12 	xul.dll 	HTMLContentSink::ProcessLINKTag 	content/html/document/src/nsHTMLContentSink.cpp:2948

That seems to be all the stacks I've looked at (random sampling of a dozen or so), in fact.

The crash is a null-pointer dereference of the aContent argument to RegisterNamedItems (and by extension ContentInserted).

We should NOT be passing null as aContent to ContentInserted.
Comment 5 Boris Zbarsky [:bz] 2009-10-29 22:55:49 PDT
Bug 502168 changed the NotifyInsert callsite in HTMLContentSink::FlushTags for 1.9.1.4 in a way that could potentially trigger this.  I bet we're getting a null |child| in some cases.  Olli, want to take a look?

Can we ship the HTML5 parser yet?  ;)
Comment 6 Olli Pettay [:smaug] 2009-10-30 05:02:42 PDT
Created attachment 409311 [details] [diff] [review]
wip

For some reason content sink handles *link* leaf tag in a such way that
sink context isn't used when appending the element to parent.
See http://mxr-test.konigsberg.mozilla.org/bonsai/cvsblame.cgi?file=content/html/document/src/nsHTMLContentSink.cpp&xrev=b67fcb2d3fe5&tree=mozilla1.9.1&mark=2428-2429,2433#2418

I need to still understand this all better, and do some testing.
Comment 7 Olli Pettay [:smaug] 2009-10-30 05:08:20 PDT
The patch is almost like the one for Bug 468562.
Comment 8 Olli Pettay [:smaug] 2009-10-30 05:12:40 PDT
Bug 468562 fixes the crash I see with http://www.hurriyet.com.tr/spor/.
And bug 468562 has been on trunk since last December, so we should probably
take it to branch(es?).
Comment 9 Olli Pettay [:smaug] 2009-10-30 05:14:30 PDT
Dbaron, you marked this as blocking1.9.2? Can you actually reproduce this
on 1.9.2?
Comment 10 XtC4UaLL [:xtc4uall] 2009-10-30 05:39:14 PDT
the crash happens if you load http://www.hurriyet.com.tr/_statikler/nesine.asp
1.9.2 seems unaffected.
Comment 11 Boris Zbarsky [:bz] 2009-10-30 08:49:55 PDT
Taking bug 468562 on 1.9.1 sounds like an excellent plan to me.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-10-30 09:36:25 PDT
(In reply to comment #9)
> Dbaron, you marked this as blocking1.9.2? Can you actually reproduce this
> on 1.9.2?

No, just wanted to make sure it's fixed there too if still a problem.
Comment 13 Samuel Sidler (old account; do not CC) 2009-10-30 10:20:58 PDT
Does this happen on 1.9.0? I'm guessing not since bug 502168 wasn't taken on that branch (and didn't apply to it, aiui).
Comment 14 Henrik Skupin (:whimboo) 2009-10-30 18:25:46 PDT
Yesterdays build of Namoroka crashed for me with this signature. So 1.9.2 is affected but as it looks like it will be fixed by bug 468562.
Comment 15 Henrik Skupin (:whimboo) 2009-10-30 18:26:30 PDT
Forgot the crash report: bp-2b794016-5082-4ae1-9fb3-df9822091030
Comment 16 Boris Zbarsky [:bz] 2009-10-30 18:46:15 PDT
Henrik, bug 468562 was fixed back in November 2008, so that fix is on 1.9.2.

The crash report in comment 15 is from a 1.9.1 build, not a 1.9.2 build.
Comment 17 Henrik Skupin (:whimboo) 2009-10-30 19:03:33 PDT
(In reply to comment #16)
> The crash report in comment 15 is from a 1.9.1 build, not a 1.9.2 build.

Oh, mixed up the running instances. Sorry. So 1.9.2 is fine and doesn't crash. A recent Grand Paradiso build also doesn't crash so 1.9.0 isn't affected.
Comment 18 Sereros 2009-11-01 09:06:41 PST
I've got a similar case: Firefox crashed when i go on www.radioplay.ch
Report: http://crash-stats.mozilla.com/report/index/e06edb46-1da2-4575-8fbc-e94b12091101?p=1
Comment 19 Samuel Sidler (old account; do not CC) 2009-11-04 17:47:37 PST
Fixed by the patch in bug 468562.
Comment 20 Al Billings [:abillings] 2009-11-05 17:08:46 PST
I don't crash on 1.9.1.4 at http://www.hurriyet.com.tr/_statikler/nesine.asp on OS X 10.6. Is this Windows only?
Comment 21 Al Billings [:abillings] 2009-11-05 17:17:33 PST
It also doesn't crash on XP and Vista on http://www.hurriyet.com.tr/_statikler/nesine.asp.
Comment 22 Daniel Veditz [:dveditz] 2009-11-05 17:33:10 PST
This is a 1.9.1-only crash. the underlying cause was fixed on 1.9.2 in the dependent bug a long time ago.
Comment 23 Henrik Skupin (:whimboo) 2009-11-06 02:31:52 PST
Yes, it's fixed now. Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 ID:20091102134505
Comment 24 Al Billings [:abillings] 2009-11-06 09:58:09 PST
Henrik, did you verify the crash happening in 1.9.1.4 before verifying that it was fixed in 1.9.1.5?
Comment 25 Henrik Skupin (:whimboo) 2009-11-06 10:05:36 PST
See my comment 14.

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