Mark Mentovai Reporter
	Description • 19 years ago

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.

	Mark Mentovai Reporter
	Comment 2 • 19 years ago

Also TB8333287, TB8386098.  Adjusting hardware/OS.

	Hermann Schwab
	Comment 3 • 19 years ago

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

	Mike Connor [:mconnor]
	Comment 4 • 19 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 5 • 19 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 19 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 7 • 19 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 8 • 19 years ago

Second problem: the HashSet is relallocted while enumerating

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 19 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 19 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 19 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 13 • 19 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 14 • 19 years ago

(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.

	Mats Palmgren (inactive) Assignee
	Comment 15 • 19 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 19 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 19 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 18 • 19 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 19 • 19 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 20 • 19 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 21 • 19 years ago

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

-> FIXED