289292 - [FIXr]crash while loading the page [@ nsContentList::IsContentAnonymous]

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Ludovic Balogh]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050406 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050406 Firefox/1.0+

Firefox frequently crash or freeze while loading
http://www.phoenixjp.net/news/fr/. This bug has been present for approximately 2
months (maybe more, I don't remember) in the trunk builds. It should load the
page whithout crashing or freezing. 

Sometimes the page load well, sometimes I have to retry 5 or more times to
succesfully load it.

When it freezes, I have to kill Firefox's process in the task manager.

Reproducible: Sometimes

Steps to Reproduce:
1. Navigate to http://www.phoenixjp.net/news/fr
Actual Results:  
Firefox crash or freeze while opening the page

Expected Results:  
Open the page without crashing or hanging

Error when it crashes :

FIREFOX.EXE - Application Error

The instruction at "0x..." referenced memory at "0x...". The memory could not be
"read".

Click on OK to terminate the program
Click on CANCEL to debug the program

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050406
Firefox/1.0+

Crashed for me on second load, new profile.

TB4884417Z

Comment 2

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.7.6) Gecko/20050321
Firefox/1.0.2
Doesn't crash with ff v1.02
+kw regression

Comment 3

[Steve England [:stevee]]

Another talkback id TB4884560W

Comment 4

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050119 Firefox/1.0+
Crashes in this (old) version too.

Comment 5

[Matthias Versen [:Matti]]

Attachment: win2k3 stack trace from 20050404 debug

Comment 6

[Matthias Versen [:Matti]]

Note: 
I get many (10+) assertions (see the attachment) before the crash in my debug
build. 
The stack in attachment 179843 [details] is from the crash and not from the assertion
above and the crash is different from the Talkback stacks.

BTW: it's not the best idea to confirm such a bug in FF:General. 
Moving to layout... 

Comment 7

[Hermann Schwab]

got some one-liners and one longer Talkback:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB4887509E

found it not crashing if JS was disabled.

working: 1.8a Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a) Gecko/20040418

crashing: BuildId 2004062123, 2004081709
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a2) Gecko/20040621
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a3) Gecko/20040817 Talkback:TB4887509E

Comment 8

[Hermann Schwab]

crashing: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a1) Gecko/20040520-09

Timeframe for regression:
1.8a  2004041809 working
1.8a1 2004052009 crashing

was looking shortly into http://archive.mozilla.org/pub/mozilla/nightly/, seems
all 1.8 based nightlys are deleted (didn´t check all)

Comment 9

[Charles Fenwick]

Seeing this on current Linux debug build, so am changing OS to 'All'.

I get lots of
 
###!!! ASSERTION: Shallow unbind won't clear document and binding parent on
kids!: 'aDeep || (!GetCurrentDoc() && !GetBindingParent())', file
/home/clfenwi/moz/mozilla/content/base/src/nsGenericElement.cpp, line 1872

before 

Program ./mozilla-bin (pid = 23298) received signal 11.

subsequent bt matches stacktrace in comment 5.

Comment 10

[Hermann Schwab]

three-day-timeframe: 

BuildId 2004042708 working
BuildId 2004042922 crashing, if JS is enabled

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-04-27+04%3A00&maxdate=2004-04-29+22%3A00&cvsroot=%2Fcvsroot

Comment 11

[Martijn Wargers (dead)]

Attachment: Testcase, zipped

Ok, this is sort of a minimal testcase.
- Unzip testcase
- Open the file named "U RL.htm"
Result: crash

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

So the problem is that the site calls document.load() a bunch of times in a row
on the same document (in a loop).  This starts of multiple loads, which then
race to complete, all parsing into the same document.  They clobber each
other's content in the document (via SetRootContent), which causes nodes to be
removed from the document (refs to them dropped) without UnbindFromTree being
called on them and without relevant nsIDocumentObserver notifications
happening.  The result is that content lists that think they're up to date are
holding pointers to deleted nodes; if they try to operate on them, they crash.

What should be done if document.load() is called on an XML document that's in
the middle of a load is an interesting question.  I see three options:

1)  Ignore the load() call.
2)  Throw an exception.
3)  Cancel the existing load and start the new one.

Option 2 would break the page in the URL bar, as far as I can tell.  Option 1
seems wrong in general.  So I went with option 3.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not too exited about the inserted loadlistener. Is it just needed for the
<parseerror> stuff? Couldn't we get hold of the sink instead and tell it to back
off from the document?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and is this really xml specific?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Is it just needed for the <parseerror> stuff?

Yes.

> Couldn't we get hold of the sink instead and tell it to back off from the
> document?

Maybe... I don't see a general way to do it, short of storing a pointer to the
sink like the HTML document does.  Perhaps we do want to just do that; we may
need it later anyway to flush when we start doing incremental XML.

> Oh, and is this really xml specific?

So far, yes (can't call Load() on other document classes).  document.open/write
has a similar effect, but is sync and writing into a currently-being-parsed
document has well-defined behavior (open() is ignored, write() inserts into the
parse stream).

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I think adding a pointer to the sink is something we should do. Not sure
even then how to tell the sink to back off though, is it enough to call cancle
and set some flag indicating that it shouldn't create the <parseerror> stuff? Or
is there a risk that there will be a few more OnDataAvailable calls from the
stream even though it's cancled?

An alternative solution might be to let the sink detect that the load was
cancled and then not insert the parseerror?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> is it enough to call cancle and set some flag indicating that it shouldn't
> create the <parseerror> stuff?

Yes.

> Or is there a risk that there will be a few more OnDataAvailable calls from
> the stream even though it's cancled?

If someone mis-implemented a necko channel, yes.  But then there's risk of it
completely ignoring cancel() too... ;)

> An alternative solution might be to let the sink detect that the load was
> cancled and then not insert the parseerror?

That's a thought.  I wonder what happens if I hit the stop button while we're
parsing XML... Let me check on that.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Without a listener

Summary of changes:

1)  Push mParser up from nsHTMLDocument to nsDocument.
2)  Terminate the parser in nsXMLDocument if we're reset while we have a
pending
    channel.
3)  Fix nsParser::Terminate to work even if called before any data has been
    parsed.

The change in nsDocument to where UnbindFromTree is called is just to make it
match the nsIContent documentation better.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 180376 [details] [diff] [review]
Without a listener

I take it calling terminate on the parser before cancling the stream is what
stops us from creating the parseerror?

r=me if i got that part right :)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yep, exactly.  That Terminate() call shuts down expat on the stream (if we've
started parsing already) and makes sure we don't call into expat when we get the
OnStopRequest from necko.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180376 [details] [diff] [review]
Without a listener

This causes some nice hangs...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the hangs

Jonas, the only difference is that this removes the mParser member of
nsHTMLDocument in addition to adding an mParser to nsDocument... ;)

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 180453 [details] [diff] [review]
Fix the hangs

sr=jst

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180453 [details] [diff] [review]
Fix the hangs

I think this is reasonably safe, for the sort of change it is, and this fixes
crashes/hangs by making sure we don't access deleted DOM nodes...

Comment 25

[chris hofmann]

Comment on attachment 180453 [details] [diff] [review]
Fix the hangs

a=chofmann

Comment 26

[Boris Zbarsky [:bzbarsky]]

Fixed