58888 - if load page offline that has SCRIPT SRC= link to online .js file, browser crashes

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P3)

Description

[ekrock's old account (dead)]

Using 11/1-09 N6 on WinNT 4.0 SP4.

To repro:
1) save to disk the simplified test case that I will attach in a moment
2) disconnect from Internet and restart browser
3) open the test case from disk
4) browser crashes

The crash appears to be caused by this line:
<script LANGUAGE="JavaScript1.1"
SRC="http://ads.web.aol.com/html/64000539/netscape?htmlpre=document.write%28%27&htmlsuf=%3CBR%3E%3CA%20HREF%3D%22http://ads.web.aol.com/link/64000539/netscape%22%3E%3CB%3EClick%20Here!%3C/B%3E%3C/A%3E%27%29%3b&xlnl=%5cn&xltick=%5c%27&ctype=application/x-javascript&cook=hdr"></script>

If you remove it, the crash goes away.

Not sure if the crash affects (a) *all* SCRIPT SRC= that try to load an online
script when you're offline, or (b) just something about this one in particular.

If (a), this bug would be an RTM limbo fix candidate, so we should investigate
ASAP to try to narrow down the cause.

Initially assigning to Parser on a guess that parser's choking on the inability
to find this, but cc:ing JS experts for input.

Comment 1

[ekrock's old account (dead)]

Attachment: simplified test case (WARNING: CRASHES BROWSER WHEN OFFLINE)

Comment 2

[ekrock's old account (dead)]

Verified that simplified test case does *not* crash browser when you're online;
a banner ad and clickable link are displayed without incident. Marking crash.

Comment 3

[ekrock's old account (dead)]

Nominating for rtm consideration, if a patch can be developed, as this crash is
being triggered by ad banner content from netscape.com (possibly used across
other AOL properties as well). It would be unfortunate to have customers saving
Netscape/AOL pages to disk, only to have those pages crash when loaded offline.

GreggL, how widespread is this SCRIPT that creates an ad banner?

Comment 4

[harishd]

Stack trace:

NS_NewStreamLoader(nsIStreamLoader * * 0x0012f244, nsIURI * 0x03b6c2d0, 
nsIStreamLoaderObserver * 0x03c6e894, nsISupports * 0x00000000, nsILoadGroup * 
0x03b21a40, nsIInterfaceRequestor * 0x03b203f4, unsigned int 512, unsigned int 
0, unsigned int 0) line 367 + 13 bytes
HTMLContentSink::ProcessSCRIPTTag(const nsIParserNode & {...}) line 4927 + 105 
bytes
HTMLContentSink::AddLeaf(HTMLContentSink * const 0x03c6e890, const nsIParserNode 
& {...}) line 3156 + 12 bytes
CNavDTD::AddLeaf(const nsIParserNode * 0x02c553c0) line 3701 + 22 bytes
CNavDTD::HandleScriptToken(const nsIParserNode * 0x02c553c0) line 2146 + 12 
bytes
CNavDTD::OpenContainer(const nsIParserNode * 0x02c553c0, nsHTMLTag 
eHTMLTag_script, int 1, nsEntryStack * 0x00000000) line 3370 + 12 bytes
CNavDTD::HandleDefaultStartToken(CToken * 0x0328f598, nsHTMLTag eHTMLTag_script, 
nsIParserNode * 0x02c553c0) line 1196 + 20 bytes
CNavDTD::HandleStartToken(CToken * 0x0328f598) line 1636 + 22 bytes
CNavDTD::HandleToken(CNavDTD * const 0x03b32410, CToken * 0x00000000, nsIParser 
* 0x03c6eec0) line 770 + 12 bytes
CNavDTD::BuildModel(CNavDTD * const 0x03b32410, nsIParser * 0x03c6eec0, 
nsITokenizer * 0x03b33030, nsITokenObserver * 0x00000000, nsIContentSink * 
0x03c6e890) line 485 + 20 bytes
nsParser::BuildModel() line 2024 + 34 bytes
nsParser::ResumeParse(int 1, int 0) line 1905 + 11 bytes
nsParser::OnDataAvailable(nsParser * const 0x03c6eec8, nsIChannel * 0x03c6b4a0, 
nsISupports * 0x00000000, nsIInputStream * 0x03f6f8c0, unsigned int 0, unsigned 
int 675) line 2357 + 19 bytes
nsDocumentOpenInfo::OnDataAvailable(nsDocumentOpenInfo * const 0x03c66890, 
nsIChannel * 0x03c6b4a0, nsISupports * 0x00000000, nsIInputStream * 0x03f6f8c0, 
unsigned int 0, unsigned int 675) line 259 + 46 bytes
nsFileChannel::OnDataAvailable(nsFileChannel * const 0x03c6b4a8, nsIChannel * 
0x03f6fe60, nsISupports * 0x00000000, nsIInputStream * 0x03f6f8c0, unsigned int 
0, unsigned int 675) line 673 + 49 bytes
nsOnDataAvailableEvent::HandleEvent(nsOnDataAvailableEvent * const 0x03f69860) 
line 400 + 47 bytes
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x03f69810) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x03f69810) line 580 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00af5a40) line 513 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00590570, unsigned int 49566, unsigned int 0, 
long 11491904) line 1049 + 9 bytes
USER32! 77e71820()
00af5a40()

Comment 5

[harishd]

Looks like somebody released the steam loader from underneath!  I think the 
problem is somewhere in Necko. Giving bug to Gagan & ccing rpotts.

Will investigate more..

Comment 6

[Brendan Eich [:brendan]]

Speaking of refcnt problems, why is loader blatantly leaked (no nsCOMPtr, no 
NS_RELEASE) after successful return from NS_NewStreamLoader at 
http://lxr.mozilla.org/mozilla/source/layout/html/document/src/nsHTMLContentSink
.cpp#4926 ?

/be

Comment 7

[harishd]

Brendan - I think this should take care of the problem your pointing.

http://lxr.mozilla.org/mozilla/source/layout/html/document/src/nsHTMLContentSink
.cpp#4722

Though, we should clarify that with Vidur.

Comment 8

[Brendan Eich [:brendan]]

harishd: duh, thanks.  I'm pre-caffeine and should shut up and read more code.

/be

Comment 9

[Brendan Eich [:brendan]]

(Trying to redeem myself, now that I'm caffeinated)  So is there a dialog of any 
kind saying "offline, can't retrieve document" ?  If so, and if NS_OpenURI fails 
when called from nsStreamLoader::Init from NS_NewStreamLoader, that dialog could 
pump the proxy event that nsStreamLoader::Init sent to call OnStreamComplete 
asynchronously, after the current callstack unwinds.

That would account for the crash.  So is there a dialog visible at any point?  
Does the offline case cause AsyncRead or similar to arrange for such a dialog?

/be

Comment 10

[vidur (gone)]

Brendan may be stimulant-deprived, but the reference counting pattern used in
the code he mentioned in HTMLContentSink is still whacky. We don't keep a
reference to the stream loader ourselves, but do increment the reference count,
decrementing it when the stream loader appears again as a parameter to a
callback. That's not the source of the problem, but should be fixed post-RTM.

Comment 11

[Johnny Stenback (:jst)]

The whacky refcounting is indeed the source of this problem, the problem is that
when we're offline an we do a NS_NewStreamLoader() we end up calling
HTMLContentSink::OnStreamComplete() synchronously from within that call and in
OnStreamComplete() we release the loadre we don't yet have a reference to and
later on we crash in necko from referencing the deleted pointer.

I and Rick Potts looked into how to fix this and the right fix is to not hold
onto the loader in the sink at all (since we don't use it in any place where
we'd need a reference to it), in stead we let necko hold on to it for us and we
simply don't release the loader in OnStreamComplete(). Doing this had a bad side
effect in necko tho, the teardown of nsAsyncStreamObserver caused the stream
loader (and the sink and lots of other stuff) to be released on the wrong thread
when we really were online and the loading of the script succeeded. So, to fix
that we hadto tweak the teardown of nsAsyncStreamObserver a bit to relase the
loader on the correct thread.

I'll attach the patch.

Comment 12

[Johnny Stenback (:jst)]

Attachment: Proposed fix

Comment 13

[Johnny Stenback (:jst)]

Attachment: Proposed fix for rtm

Comment 14

[Johnny Stenback (:jst)]

The second patch in this bug is a proposed fix for the netscape branch, it's
smaller and less complicated and it does fix the problem but it's not as elegant
as the first patch in the bug. I'd proposed to get the second patch in for rtm
and then land the first patch on the trunk. r= or sr= anyone?

Comment 15

[rpotts (gone)]

I'll sr=rpotts both patches :-)

The first patch is "better" in the long run, since I believe that it will fix 
other threadsafety bugs out there :-)  It would be great to get this one on the 
trunk as soon as possible...

The second patch also fixes the problem, but does not have the far-reaching 
ramifications (or benifits) of fixing the nsAsyncStreamListener...

-- rick

Comment 16

[vidur (gone)]

I'd go with the HTMLContentSink patch. It's more localized and IMO closer to
Zaro Reesk. r=vidur for that one.

Comment 17

[Brendan Eich [:brendan]]

So there was a synchronous call to OnStreamComplete hidden in an error case
reached from the NS_NewStreamLoader call -- jst told me on the phone that it was
in AsyncRead.  This seems like a separate bug that should be filed, if it isn't
already filed.  Rick, does that sound right?

/be

Comment 18

[selmer (gone)]

rtm-, not ship stopper.

Comment 19

[Gagan]

->jst

Comment 20

[Johnny Stenback (:jst)]

Fix checked in on the trunk earlier today, before checkin in I realized that the
problem in the HTML content sink also exists in the XML content sink so I made
the same change there before checking it in...

Comment 21

[rpotts (gone)]

hey brendan,

It sounds like we just need to define the semantics of the OnStreamComplete(...) 
notification...  

I believe that there are other failure situations, where notifications can be 
fired before the "constructor function" has returned...

I'm not sure if this is evil, as long as it is well documented and people know 
what to expect :-)

-- rick

Comment 22

[Johnny Stenback (:jst)]

*** Bug 59256 has been marked as a duplicate of this bug. ***

Comment 23

[bsharma]

updated qa contact.

Comment 24

[bsharma]

Verified on:
build: 2001-03-29-09-Mtrunk
Platform: Win NT

The attached test case does not crashes when online or offline.