304747 - Crash [@ nsQueryInterface::operator()]

Status: VERIFIED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Mark Mentovai]

In Firefox, trunk/1.8 branch: at http://www.meriweather.com/777/over/elect.html
, click on the BATTERY button in the image map.  You expect a box to pop up in
the document via JavaScript.  Instead, Firefox crashes.  The crash is in
nsQueryInterface::operator(), called by nsContentUtils::GetLinkURI.  A more
complete backtrace will be attached.

This bug is not present in the Mac 081008 build, and is present in the Mac
081108 build.  I have not tested other platforms personally, but am told that it
does not occur in a Win 0812 build.  Regression window:

http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2005-08-10+08%3A00&maxdate=2005-08-11+09%3A00

Most likely cause, bug 78510.

This bug does not seem to appear if the page is already in history (or cache?),
bolstering the suspicion of bug 78510.  Because of this, the bug is most easily
reproduced with a clean profile.  Using a clean profile, it is consistently
reproducible.

Talkback IDs TB8392222Z, TB8392333M.  They occurred on different sites, but have
identical stack signatures.

Comment 1

[Mark Mentovai]

Attachment: Stack backtrace

Comment 2

[Mark Mentovai]

Also TB8333287, TB8386098.  Adjusting hardware/OS.

Comment 3

[Hermann Schwab]

It doesn't crash in a Win 0811 build or Win 0813 build.

Bug 304639 History/frameset related (?) crash
has same (10/11) timeframe for regression

Comment 4

[Mike Connor [:mconnor]]

Roc, can you take a look at this? Seems to be yours.

Comment 5

[Mats Palmgren (inactive)]

It's a re-entrancy problem on nsDocument::mLinkMap...

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I know, I'm working on it :-)

Comment 7

[Mats Palmgren (inactive)]

Attachment: Stack 1

First problem: mLinkMap is reallocated while enumerating in 
nsUint32ToContentHashEntry::VisitContent

Comment 8

[Mats Palmgren (inactive)]

Attachment: Stack 2

Second problem: the HashSet is relallocted while enumerating

Comment 9

[Mats Palmgren (inactive)]

Attachment: Like so?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What happens is that when enumerating the table to mark links visited, we set
the link state to unknown and then tell the style system about it. The style
system rechecks the link state, which causes us to add the link to the table as
a new style-relevant link.

This shouldn't be reallocating the table, however, since the link is already in
the table.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's OK, except you forgot to add to the list

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think your patch is the way to go, but I'd like to understand *why* the
reallocation is happening.

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix

Oops, that got lost when I cleaned out the tracing stuff...

Comment 14

[Mats Palmgren (inactive)]

(In reply to comment #12)
> I think your patch is the way to go, but I'd like to understand *why* the
> reallocation is happening.

Look in stack 2, since we add stuff through AddStyleRelevantLink (which calls
PutContent) while having the enumeration on the stack (frame #15) we will
eventually crash when the "backing store" for those structures are reallocted
to some other place in memory.

Comment 15

[Mats Palmgren (inactive)]

(This bug seems unrelated to bug 304639 / bug 303606 BTW)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, but since the link element being added is already in the map, adding it
again shouldn't be doing anything.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Okay, pldhash can indeed reallocate the table when adding an element that
already exists. "blargh".
http://lxr.mozilla.org/mozilla/source/xpcom/glue/pldhash.c#506

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 192892 [details] [diff] [review]
fix

For safety's sake, I think this should be an array of strong references
(nsISupportsArray). We don't want one of the content elements to go away
somehow if a ContentStatesChanged callback removes elements from the document
and map.

Comment 19

[Mats Palmgren (inactive)]

Attachment: Patch 3

I used nsCOMArray<nsIContent> hope that's ok.
Regarding removed content - do you think we need to check "IsInDoc()" before
calling ContentStatesChanged() also?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 192899 [details] [diff] [review]
Patch 3

if we need to bulletproof for IsInDoc then the right place to do that is inside
ContentStateChanged.

this will fix a branch crasher.

Comment 21

[Mats Palmgren (inactive)]

Checked in to trunk and 1.8 branch at 2005-08-18 02:54 PDT

-> FIXED