Closed
Bug 218031
Opened 21 years ago
Closed 21 years ago
[FIXr]crash [@ MathMLElementFactoryImpl::CreateInstanceByTag] opening MathML + CSS page
Categories
(Core :: MathML, defect, P1)
Core
MathML
Tracking
()
RESOLVED
FIXED
mozilla1.6alpha
People
(Reporter: hal, Assigned: bzbarsky)
References
()
Details
(Keywords: crash, regression, testcase)
Crash Data
Attachments
(2 files)
5.17 KB,
text/plain
|
Details | |
12.93 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
Confirmed on trunk linux from cvs. Stack coming shortly. -> MathML
Assignee: general → rbs
Status: UNCONFIRMED → NEW
Component: Browser-General → MathML
Ever confirmed: true
QA Contact: general → ian
Comment 2•21 years ago
|
||
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•21 years ago
|
||
+crash, +regression (tentative) based on claim that it worked in 1.4 updating summary
Keywords: crash,
regression
Summary: opening this page crashes mozilla 1.5 repeatedly, from either Linux or Windows. It works fine with Mozilla 1.4 → crash [@ MathMLElementFactoryImpl::CreateInstanceByTag] opening MathML + CSS page
Comment 4•21 years ago
|
||
TB23257247Z Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030828
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•21 years ago
|
||
cf bug 217674, where a sheet with a null uri was causing IsChromeURI to barf.
Comment 7•21 years ago
|
||
regression between linux trunk 2003071605 and 2003071805, probably bug 212504
Keywords: testcase
Assignee | ||
Comment 8•21 years ago
|
||
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.
Blocks: 217674
Comment 9•21 years ago
|
||
*** Bug 221265 has been marked as a duplicate of this bug. ***
Comment 11•21 years ago
|
||
Passing the hot potato to bz. I am an observer here. It is regression from bz's bug 96858.
Assignee: rbs → bzbarsky
Assignee | ||
Comment 12•21 years ago
|
||
Sorry I lost track of this bug...
Assignee | ||
Comment 13•21 years ago
|
||
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).
Attachment #132711 -
Flags: superreview?(dbaron)
Attachment #132711 -
Flags: review?(dbaron)
Assignee | ||
Updated•21 years ago
|
Priority: -- → P1
Summary: crash [@ MathMLElementFactoryImpl::CreateInstanceByTag] opening MathML + CSS page → [FIX]crash [@ MathMLElementFactoryImpl::CreateInstanceByTag] opening MathML + CSS page
Target Milestone: --- → mozilla1.6alpha
Attachment #132711 -
Flags: superreview?(dbaron)
Attachment #132711 -
Flags: superreview+
Attachment #132711 -
Flags: review?(dbaron)
Attachment #132711 -
Flags: review+
Assignee | ||
Updated•21 years ago
|
Summary: [FIX]crash [@ MathMLElementFactoryImpl::CreateInstanceByTag] opening MathML + CSS page → [FIXr]crash [@ MathMLElementFactoryImpl::CreateInstanceByTag] opening MathML + CSS page
Assignee | ||
Comment 14•21 years ago
|
||
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....
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 15•21 years ago
|
||
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.
Assignee | ||
Comment 16•21 years ago
|
||
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•21 years ago
|
||
*** Bug 221092 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 18•21 years ago
|
||
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...
Attachment #132711 -
Flags: approval1.5?
Updated•21 years ago
|
Flags: blocking1.5?
Comment 19•21 years ago
|
||
Comment on attachment 132711 [details] [diff] [review] This fixes the crash 1.5 shipped. removing obsolete request.
Attachment #132711 -
Flags: approval1.5?
Comment 20•21 years ago
|
||
*** Bug 223532 has been marked as a duplicate of this bug. ***
Comment 21•21 years ago
|
||
*** Bug 217674 has been marked as a duplicate of this bug. ***
Comment 22•21 years ago
|
||
*** Bug 225187 has been marked as a duplicate of this bug. ***
Comment 23•21 years ago
|
||
*** Bug 230896 has been marked as a duplicate of this bug. ***
Updated•13 years ago
|
Crash Signature: [@ MathMLElementFactoryImpl::CreateInstanceByTag]
You need to log in
before you can comment on or make changes to this bug.
Description
•