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

VERIFIED FIXED

Status

()

Core
HTML: Parser
P3
critical
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: ekrock's old account (dead), Assigned: jst)

Tracking

({crash})

Trunk
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm-])

Attachments

(3 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 18548 [details]
simplified test case (WARNING: CRASHES BROWSER WHEN OFFLINE)
(Reporter)

Comment 2

17 years ago
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.
Keywords: crash
Summary: if load page offline that as SCRIPT SRC= link, browser crashes → if load page offline that has SCRIPT SRC= link to online .js file, browser crashes
(Reporter)

Comment 3

17 years ago
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?
Keywords: rtm

Comment 4

17 years ago
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()
Status: NEW → ASSIGNED

Comment 5

17 years ago
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..
Assignee: harishd → gagan
Status: ASSIGNED → NEW
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

17 years ago
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.
harishd: duh, thanks.  I'm pre-caffeine and should shut up and read more code.

/be
(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

17 years ago
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.
(Assignee)

Comment 11

17 years ago
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.
(Assignee)

Comment 12

17 years ago
Created attachment 18594 [details] [diff] [review]
Proposed fix
(Assignee)

Comment 13

17 years ago
Created attachment 18595 [details] [diff] [review]
Proposed fix for rtm
(Assignee)

Comment 14

17 years ago
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

17 years ago
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

17 years ago
I'd go with the HTMLContentSink patch. It's more localized and IMO closer to
Zaro Reesk. r=vidur for that one.
Whiteboard: [rtm+]
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

17 years ago
rtm-, not ship stopper.
Whiteboard: [rtm+] → [rtm-]

Comment 19

17 years ago
->jst
Assignee: gagan → jst
(Assignee)

Comment 20

17 years ago
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...
Status: NEW → RESOLVED
Last Resolved: 17 years ago
OS: Windows NT → All
Hardware: PC → All
Resolution: --- → FIXED

Comment 21

17 years ago
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
(Assignee)

Comment 22

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

Comment 23

17 years ago
updated qa contact.
QA Contact: janc → bsharma

Updated

17 years ago
Status: RESOLVED → VERIFIED

Comment 24

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

The attached test case does not crashes when online or offline.
You need to log in before you can comment on or make changes to this bug.