220542 - document.write of <link> with invalid protocol crash [@ CNavDTD::BuildModel]

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[António Ferreira]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20030925
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20030925

Mozilla craches with the html code in the URL http://flop.catus.net/mozbug/
I’ve tried mozilla 1.4 final and 1.5rc2.

The html code on that URL is this:
<script>document.write('<link href="l:\\" rel=stylesheet>')</script>

Reproducible: Always

Steps to Reproduce:
1. Open the browser on the URL http://flop.catus.net/mozbug/
2. Wait for it to load the page and crach

Actual Results:  
It just craches...

Expected Results:  
nothing...

Comment 1

[u63580]

Are either of these related: bug 202765 and bug 185285?

Comment 2

[Matthias Versen [:Matti]]

Attachment: win2k stack trace

Comment 3

[Matthias Versen [:Matti]]

-> Parser

Comment 4

[Mats Palmgren (inactive)]

Bug occurs in Mozilla 1.3.1, 1.4, 1.5rc1 and trunk on Linux.
Bug does not occur in Mozilla 1.2.1 and 1.0.2

Comment 5

[Harshal Pradhan]

bz: random guess, might we be incorrectly blocking/unblocking the parser for
sheets with bogus urls ?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah.  We're blocking the parser for the script, then starting the sheet load,
which fails to open the channel in LoadSheet() and unblocks the parser, and then
things are all broken.  This could be wallpapered over with a lot of effort, but
it's better to just focus on bug 84582.

Comment 7

[Vidar Haarr (not reading bugmail)]

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

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 10

[Martijn Wargers (dead)]

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

Comment 11

[Matthias Versen [:Matti]]

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

Comment 12

[Brendan Eich [:brendan]]

This bug is not well-assigned.  Anyone (mrbkap!) want to take it?

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

With this patch, I introduce a new function to the nsIParser interface:
ContinueInterruptedParsing(). This function is to be used when the parser is
not currently parsing anything (not enforced) but is unblocked. This separation
allows me to stop any attempts to ContinueParsing() on a parser that is
unblocked, which in common usage means specifically that we are indeed already
parsing. I'm submitting this as a wallpaper until bug 84582 is fixed.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 171101 [details] [diff] [review]
patch v1

r=jst

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 171101 [details] [diff] [review]
patch v1

Looking for sr. If this is to be checked in for beta, then it'd be good to get
it checked in soon so we can catch regressions.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 171101 [details] [diff] [review]
patch v1

Yikes, this doesn't work quite right, I forgot to change the failed script load
case to call ContinueInterruptedParsing(). I'll attach a new patch, but hold
off on requesting reviews (since it looks like the approach in bug 243338 is
the safer fix).

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 171431 [details] [diff] [review]
patch v2

Actually, I now believe this patch is safe enough to be checked in.

There are currently 4 calls to nsParser::ContinueParsing(). One of these calls
is from nsParser::HandleParserContinueEvent().	This can safely call
ContinueInterruptedParsing(). Two other calls are in the script loader. One of
them is preceeded by code that says:
if (!mParser->IsParserEnabled()) {
  mParser->UnblockParser();
}

The other checks for IsParserEnabled() before calling ContinueParsing(), so
changing both of these to ContinueInterruptedParsing() is guaranteed to be
safe.

The only questionable call is the one in the css loader. However, there are two
paths to getting to the call:
1) The CSS loaded (or failed to connect, etc.) and the parser was blocked by
the loader. In this case ContinueParsing() is the correct thing to do.
2) There was a problem with the URI and we reached the call directly from the
parser without blocking it. In this case, we *should* ignore the
ContinueParsing() request and all the flags will be set correctly to do so.

In light of this, asking for sr=.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 171431 [details] [diff] [review]
patch v2

>     // Call this method to resume the parser from the blocked state..
>     NS_IMETHOD ContinueParsing() = 0;
>+    // Call this method to resume the parser from an unblocked state.
>+    NS_IMETHOD ContinueInterruptedParsing() = 0;

How would we be in such a state?  That is, requiring resuming but not blocked?

Perhaps some documentation on the interaction of these two functions with
BlockParser and UnblockParser (with examples, as needed) is in order.

sr=bzbarsky with some documentation about this.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch for checkin

Comment 21

[Boris Zbarsky [:bzbarsky]]

Note that I removed the semicolons after the IIDs in nsIParser.h to fix build
bustage.

Fix checked in.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Fix was checked in, marking this bug FIXED.

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED with build 2005-01-28-04 on Windows XP using Seamonkey trunk.

Tested with both http://flop.catus.net/mozbug/ and
data:text/html,<script>document.write('<link href="l:\\"
rel=stylesheet>')</script> (inserted into this bug's URL textfield).

Comment 24

[rbs]

I am hitting this assertion:

###!!! ASSERTION: Trying to continue parsing on a unblocked parser.: '!(mFlags &
 NS_PARSER_FLAG_PARSER_ENABLED)', file d:/Mozilla/tip/mozilla/parser/htmlparser/
src/nsParser.cpp, line 1417

It is not easily reproducible, but seems to happen when clicking Reload on a
page with stylesheet links that take time to complete (I got the assertion at:
http://www.mozilla.org/start/)

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #24)
> It is not easily reproducible,

The next time you see this, could you please attach a stack trace? Incidentally
When this happens, does the page finish loading (i.e., does the throbber stop
spinning)? If it does, I think this may be beign.

Comment 26

[Mats Palmgren (inactive)]

Attachment: Call stack for ASSERTION

Call stack for:
###!!! ASSERTION: Trying to continue parsing on a unblocked parser.: '!(mFlags
& NS_PARSER_FLAG_PARSER_ENABLED)', file nsParser.cpp, line 1417

It is 100% reproducable for me at http://flop.catus.net/mozbug/
(and the throbber stops spinning)

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #26)
> It is 100% reproducable for me at http://flop.catus.net/mozbug/
> (and the throbber stops spinning)

This is, unfortunately, expected. The CSS loader needs to be smarter about when
it calls ContinueParsing() and the assertion is a reminder of this. The reason
that that testcase asserts is because of the invalid <link> document.write()'n
out. The reason I'm concerned about rbs' assertion is because it's on a page
that doesn't have such a problem. There's a partial patch for the assertion on
this page in bug 243338.

Comment 28

[Brendan Eich [:brendan]]

Assertions should be fatal, and will be in 1.9 if I can help it, so do not use
them as warnings.  If your code can cope with this assertion botching, you
should not be asserting at all.  Please use NS_WARNING.

/be

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 30

[Adam Guthrie]

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

Comment 31

[Daniel Veditz [:dveditz]]

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

Comment 32

[Bob Clary [:bc] (inactive)]

parser/htmlparser/tests/crashtests/220542-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3