Closed Bug 241300 Opened 20 years ago Closed 20 years ago

Crash with with firefox on this url with invalid background [@ nsImageLoader::Load ]

Categories

(Core :: Layout, defect, P1)

x86
Windows 2000
defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

References

()

Details

(4 keywords)

Crash Data

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040420 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040420 Firefox/0.8.0+

From the Mozillazine forums:
http://forums.mozillazine.org/viewtopic.php?t=70726&sid=4913fcda6a319ed8b94887ccfa1fed85

This url is reliably crashing Firefox (not Mozilla).

The minimal testcase is this:
<html><head></head>
<body background="cid:00d201c264d0$feb75c80$0300a8c0@node3">
</body>
</html> 

Not crashing in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040305
Firefox/0.8.0+

Crashing in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040309
Firefox/0.8.0+

Reproducible: Always
Steps to Reproduce:
1.Visit the url
2.
3.

Actual Results:  
Crash

Expected Results:  
No crash
Attached file This will crash!
Sorry, I forgot to mention that for the testcase to crash, you have to resize
the window.
Martijn: Could you provide TalkBack incident ID?
Severity: normal → critical
Keywords: crash
Keywords: regression
(In reply to comment #3)
> Martijn: Could you provide TalkBack incident ID?
Sorry, no, afaik there are yet no firefox builds on windows with Talkback
enabled. However, I heard there are Talkback enabled builds on Linux.
I've got a linux build with talkback, but I can't reproduce the bug...
WorksForMe with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040415
Firefox/0.8.0+
i opened the bug in a new tab (without resizing anything) and it DID crash. I
use 20040331 nighty.
Confirming and marking new : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8a) Gecko/20040416 Firefox/0.8.0+

Status: UNCONFIRMED → NEW
Ever confirmed: true
I've been able to compile a firefox debug build.

Just before the crash I get an assertion (with the testcase and the url):
You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr!=0', file
../../../../dist/include/xpcom/nsCOMPtr.h line 711
Confirmed crash on testcase when resizing window.

- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040503
Firefox/0.8.0+
- Microsoft Windows 2000 Pro 5.00.2195 SP4
The url given WFM

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a) Gecko/20040517
Firefox/0.8.0+
Well, my 2004/05/12 Firefox build seems to have Talkback enabled.
This is the Talkback ID: TB52084G
With my debug Firefox build from 2004/05/17 I don't get an assertion anymore,
but still a crash.

The GDB console shows me this:
Program received signal SIGSEGV, Segmentation fault.
nsImageLoader::Load(imgIRequest*) (this=0x83024b0, aImage=0x7fcf320) at
../../../dist/include/xpcom/nsCOMPtr.h:710

The second time, this:
nsImageLoader::Load(imgIRequest*) (this=0x7eda8e8, aImage=0x7d19ff0) at
../../../dist/include/xpcom/nsCOMPtr.h:710
wfm FF 20040604 trunk on Linux and Seamonkey debug build 20040604. Anyone can
check if still crashing and attach a full stacktrace or Talkback ID ?
Summary: Crash with with firefox on this url with invalid background → Crash with with firefox on this url with invalid background [@ nsImageLoader::Load ]
It still crashes with me:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/2004-06-06
Firefox/0.8.0+
No Talkback agent is bundled with the nightlies, so I can't give a Talkback ID.
With the debug build, I still get the same gdb messages.

The testcase doesn't crash that easily. The most likely circumstance in which it
crashes:
-Save it locally.
-Open it and press the 'Go'-button a few times.
It crashes for me using Firefox 0.9.1 (inst. build, WinXP SP1/current,
Talkback-enabled, no extension, no theme)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1

I cannot give any TB ID however. Is it because I'm behind an HTTP proxy ?

Here's what I did
- save the testcase locally. Quit Fx.
- launch Fx, type the URL file:///d:/tmp/bug_241300.htm and [RETURN].
- Click the URL to select it anew. [RETURN] a 2nd time. Crash.
- Talkback appears, Send, TB exits (no TB error message, no ID).
Martijn: To get the Talkback ID's go to your Firefox directory and go to
components/, there start talkback.exe, it'll give you a list with the IDs.
Thanks Adam, indeed I have the ID now. (I assume that your comment was addressed
to me.) I did just the same, this time with 0.9.1-release.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1

There's an interesting thing (I think). I recall the sequence :
1. Launch Fx, type the local URL for the testcase, [RETURN]. Blank page.
2. Click to select the URL just typed, [RETURN]. Nothing happens. Again a blank
page.
3. Click on it (the blank page), just to see. Nothing happens. But ...
4. Click on the URL bar, as if I wanted to repeat step 2, and there's the crash !
The crash does not happen right after step 2.

Talkback ID TB193939W 2004-06-29 23:24 GMT+02 , sent.
David, Boris: This looks like regression from bug 57607 changes (regression
range is between 20040305 and 20040309, check-in is 2004-03-08). What do you think?
*** Bug 248315 has been marked as a duplicate of this bug. ***
This bug also affects Thunderbird - see bug 248315.
-> Layout
QA Contact: firefox.general
Assignee: firefox → nobody
Component: General → Layout
Product: Firefox → Browser
QA Contact: firefox.general → core.layout
Version: unspecified → Trunk
Attached patch Maybe this? (obsolete) — Splinter Review
I don't know if this is right, but this makes the testcase not crash anymore.
     nsCOMPtr<nsIURI> oldURI;
+    if (!oldURI)
+      return NS_ERROR_FAILURE;

that is wrong. there is _never_ a case where oldURI is non-null at this point.
Comment on attachment 146752 [details]
This will crash!

This testcase doesn't crash for me in Linux Firefox on either the Trunk or the
Aviary 1.0 branch.
Do you have to have "cid:whatever" to crash?  Does using "foo:bar" crash also? 
If not, does it matter what comes after the "cid:" part?
(In reply to comment #24)
> Do you have to have "cid:whatever" to crash?  Does using "foo:bar" crash also? 
> If not, does it matter what comes after the "cid:" part?
"cid:foo" does indeed crash (the new url's testcase crashes), but "foo:bar" does
not crash, so I don't think it does matter much what comes after the "cid:" part.

This is the output from the command 'bt' of gdb:

#0  0x77f813b2 in ?? ()
#1  0x67834260 in nsDebugImpl::Break(char const*, int) (this=0x198be80,
    aFile=0x36fefa0 "../../../dist/include/xpcom/nsCOMPtr.h", aLine=711)
    at c:/mozilla/mozilla/xpcom/base/nsDebugImpl.cpp:306
#2  0x6783416f in nsDebugImpl::Assertion(char const*, char const*, char const*,
int) (this=0x198be80,
    aStr=0x36fefe0 "You can't dereference a NULL nsCOMPtr with operator->().", a
Expr=0x36fefc7 "mRawPtr != 0",
    aFile=0x36fefa0 "../../../dist/include/xpcom/nsCOMPtr.h", aLine=711)
    at c:/mozilla/mozilla/xpcom/base/nsDebugImpl.cpp:286
#3  0x6785d8d6 in nsDebug::Assertion(char const*, char const*, char const*, int)
 (aStr=0x36fefe0 "You can't dereference a NULL nsCOMPtr with operator->().",
    aExpr=0x36fefc7 "mRawPtr != 0",
    aFile=0x36fefa0 "../../../dist/include/xpcom/nsCOMPtr.h", aLine=711)
    at c:/mozilla/mozilla/xpcom/glue/nsDebug.cpp:108
#4  0x036ff3fa in nsImageLoader::Load(imgIRequest*) (this=0xf513918,
    aImage=0xf45e548) at ../../../dist/include/xpcom/nsCOMPtr.h:711
#5  0x03705130 in nsPresContext::LoadImage(imgIRequest*, nsIFrame*, imgIRequest*
*) (this=0xf4ea6e0, aImage=0xf45e548, aTargetFrame=0xed68ea4,
    aRequest=0x22e640)
    at c:/mozilla/mozilla/layout/base/src/nsPresContext.cpp:985
#6  0x0366d6d0 in nsCSSRendering::PaintBackgroundWithSC(nsIPresContext*, nsIRend
eringContext&, nsIFrame*, nsRect const&, nsRect const&, nsStyleBackground const&
, nsStyleBorder const&, nsStylePadding const&, int, nsRect*) (
    aPresContext=0xf4ea6e0, aRenderingContext=@0xed41388,
    aForFrame=0xed68ea4, aDirtyRect=@0x22e9a0, aBorderArea=@0x22e780,
    aColor=@0x22e6f0, aBorder=@0xee42618, aPadding=@0xee42b40,
    aUsePrintSettings=1, aBGClipRect=0x0)
    at ../../../../dist/include/xpcom/nsCOMPtr.h:704
#7  0x0366cf05 in nsCSSRendering::PaintBackground(nsIPresContext*, nsIRenderingC
ontext&, nsIFrame*, nsRect const&, nsRect const&, nsStyleBorder const&, nsStyleP
adding const&, int, nsRect*) (aPresContext=0xf4ea6e0,
    aRenderingContext=@0xed41388, aForFrame=0xed68ea4, aDirtyRect=@0x22e9a0,
    aBorderArea=@0x22e780, aBorder=@0xee42618, aPadding=@0xee42b40,
    aUsePrintSettings=1, aBGClipRect=0x0)
    at c:/mozilla/mozilla/layout/html/style/src/nsCSSRendering.cpp:2787

I can give more of this output, if needed.
What are the values of the locals and member variables in frame 4?
Attached patch Proposed patchSplinter Review
Martijn, thanks for the debug data!

Could someone seeing the crash try this patch?
Attachment #152193 - Attachment is obsolete: true
Attachment #153222 - Flags: superreview?(darin)
Attachment #153222 - Flags: review?(cbiesinger)
Comment on attachment 153222 [details] [diff] [review]
Proposed patch

     NS_NEWXPCOM(channel, nsExtProtocolChannel);
     if (!channel) return NS_ERROR_OUT_OF_MEMORY;

     ((nsExtProtocolChannel*) channel.get())->SetURI(aURI);
+    ((nsExtProtocolChannel*) channel.get())->SetOriginalURI(aURI);

ideally this'd be constructor arguments...

But in any case, setting originalURI does not need a cast.
Attachment #153222 - Flags: review?(cbiesinger) → review+
(In reply to comment #28)
> Could someone seeing the crash try this patch?
I've applied the patch and did a rebuild and now I don't see a crash anymore
with the testcase.
Comment on attachment 153222 [details] [diff] [review]
Proposed patch

>Index: uriloader/exthandler/nsExternalProtocolHandler.cpp

> NS_IMETHODIMP nsExtProtocolChannel::GetOriginalURI(nsIURI* *aURI)
> {
>+  NS_PRECONDITION(aURI, "Must have original URI!");
>+  NS_IF_ADDREF(*aURI = mOriginalURI);
>   return NS_OK; 
> }

i'd probably remove the NS_PRECONDITION since it doesn't seem
to add much.  we'd crash if we hit that assertion, and it would
be pretty obvious that the crash occured because the input arg
was null.  also the text of the assertion doesn't seem to make
sense to me.
Attachment #153222 - Flags: superreview?(darin) → superreview+
Assignee: nobody → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
Checked in, with review comments addressed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
*** Bug 254907 has been marked as a duplicate of this bug. ***
This didn't make the Aviary branch apparently. See bug 250917 (which can
probably be duped here)
Flags: blocking-aviary1.0?
*** Bug 250917 has been marked as a duplicate of this bug. ***
Comment on attachment 153222 [details] [diff] [review]
Proposed patch

This ought to be safe enough for the branches...
Attachment #153222 - Flags: approval1.7.x?
Attachment #153222 - Flags: approval-aviary?
Comment on attachment 153222 [details] [diff] [review]
Proposed patch

a=dveditz for 1.7.x
Attachment #153222 - Flags: approval1.7.x? → approval1.7.x+
Comment on attachment 153222 [details] [diff] [review]
Proposed patch

a=asa for aviary checkin.
Attachment #153222 - Flags: approval-aviary? → approval-aviary+
Fixed on branches.
Flags: blocking-aviary1.0?
Flags: in-testsuite+
Crash Signature: [@ nsImageLoader::Load ]
You need to log in before you can comment on or make changes to this bug.