218031 - [FIXr]crash [@ MathMLElementFactoryImpl::CreateInstanceByTag] opening MathML + CSS page

Status: RESOLVED FIXED

(Core :: MathML, defect, P1)

Description

[Hal Abelson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030827
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030827

This looks like it's due to some interaction between Mathml and CSS.  Eliminting
the link to the style sheet makes things work.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Actual Results:  
This causes a repeatable crash. Simply try to open the page and the crash should
occur

Expected Results:  
This works fine in Mozilla 1.4

gklayout.dll

Comment 1

[Chase Tingley]

Confirmed on trunk linux from cvs.  Stack coming shortly.
-> MathML

Comment 2

[Chase Tingley]

Attachment: stacktrace

0x41333841 in MathMLElementFactoryImpl::CreateInstanceByTag(nsINodeInfo*,
nsIContent**) (this=0x87346c8, aNodeInfo=0x8735800, aResult=0xbfffe864)
    at nsXMLContentSink.cpp:1639
1639		    uri->GetSpec(uriStr);

stylesheet pointer looks ok, but |uri| is null.

Comment 3

[Chase Tingley]

+crash, +regression (tentative) based on claim that it worked in 1.4
updating summary

Comment 4

[Hermann Schwab]

TB23257247Z Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030828

Comment 5

[rbs]

Crashing because the document is saying that it has a stylesheet without an URI
for that stylesheet. Might be possible to fix (or should I say wall-paper) with
a null check. But how come this situation now happens in the Style system?

Comment 6

[Chase Tingley]

cf bug 217674, where a sheet with a null uri was causing IsChromeURI to barf.  

Comment 7

[Andrew Schultz]

regression between linux trunk 2003071605 and 2003071805, probably bug 212504

Comment 8

[Boris Zbarsky [:bzbarsky]]

Actually, this is a regression from bug 96858.  In this case the link to the
sheet is a 404, so CSSLoaderImpl::ParseSheet is never called.  Thus the sheet is
never inited with its URI and has a null URI.

One solution is to do the following:
1)  Remove the call to Init() from ParseSheet.
2)  Add calls to Init() in all sorts of places (LoadInlineStyle, LoadSheet,
    SheetLoadData::OnStreamComplete, lots of error cases, etc).

A second solution is to make it possible to change the URI of an existing sheet
(probably only while it's incomplete and has no rules) and then init sheets at
creation-time as we used to and just change the URI as needed in OnStreamComplete.

A third solution is to have a more useful indication of error/success given to
SheetComplete (a bitfield instead of a boolean) so that it can decide whether
the sheet needs initing (fragile, imo, though I may want that bitfield for other
reasons).

I think #2 is the one to go with.  David?  What do you think?

I have no useful CVS access till the 23rd or 24th, so I'm afraid someone will
need to produce a patch...

Bug 217674 is almost certainly caused by the same problem as this bug.

Comment 9

[Harshal Pradhan]

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

Comment 10

[Alan Eliasen]

Nominating 1.5 blocker

Comment 11

[rbs]

Passing the hot potato to bz. I am an observer here. It is regression from bz's
bug 96858.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes the crash

Sorry I lost track of this bug...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132711 [details] [diff] [review]
This fixes the crash

David, could you review?  This basically backs out the CSSLoader parts of the
patch in bug 96858 and refixes it in a different way (by calling SetURL when we
have the final post-redirect URI).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in.  To be truthful, I'm not sure about whether this should go on the
1.5 branch; in any case, it should bake on trunk for a bit first....

Comment 15

[rbs]

Since bug 96858 forgot about 404 urls -- something very common (especially
during web page development), it seems to me that it is worth checkin the fix in
the branch. Considering moreover your earlier comment that:

> Bug 217674 is almost certainly caused by the same problem as this bug.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Let's have this bake on the trunk for a day or two, then see how things look. 
I've spent some time thinking about this patch, and I'm more and more convinced
that it's safe for 1.5.  If drivers are afraid, though, I can post a minimally
invasive change that will not catch all error cases (eg probably not bug
217674), but will catch the 404 problem...

Comment 17

[Charles Fenwick]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132711 [details] [diff] [review]
This fixes the crash

OK, this seems to be happy on the trunk... Could we take this on the branch
too?

If this is deemed too scary I can try to come up with a branch-only patch to
wallpaper over some cases of the crash...

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 132711 [details] [diff] [review]
This fixes the crash

1.5 shipped. removing obsolete request.

Comment 20

[rbs]

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

Comment 21

[timeless]

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

Comment 22

[rbs]

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

Comment 23

[Mike Connor [:mconnor]]

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