657191 - When SVG background hits HTTP 500: crash [@ mozilla::imagelib::VectorImage::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int)] | ASSERTION: You can't dereference a NULL nsRefPtr with operator->().: 'mRawPtr != 0'

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://maggie.idium.no/ntg.no/
2. crash winxp/mac at least nightly and 4.0.1

###!!! ASSERTION: You can't dereference a NULL nsRefPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/nsAutoPtr.h, line 1117

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x04d9c73e in mozilla::imagelib::VectorImage::OnDataAvailable (this=0x25153ac0, aRequest=0xd2fe34, aCtxt=0x0, aInStr=0x251176e0, aSourceOffset=0, aCount=1090) at /work/mozilla/builds/2.0.0/mozilla/modules/libpr0n/src/VectorImage.cpp:701
701	                                              aSourceOffset, aCount);
(gdb) bt
#0  0x04d9c73e in mozilla::imagelib::VectorImage::OnDataAvailable (this=0x25153ac0, aRequest=0xd2fe34, aCtxt=0x0, aInStr=0x251176e0, aSourceOffset=0, aCount=1090) at /work/mozilla/builds/2.0.0/mozilla/modules/libpr0n/src/VectorImage.cpp:701
#1  0x04db43c8 in imgRequest::OnDataAvailable (this=0x251ddd30, aRequest=0xd2fe34, ctxt=0x0, inStr=0x251176e0, sourceOffset=0, count=1090) at /work/mozilla/builds/2.0.0/mozilla/modules/libpr0n/src/imgRequest.cpp:1158
#2  0x04da2a6b in ProxyListener::OnDataAvailable (this=0x251ddac0, aRequest=0xd2fe34, ctxt=0x0, inStr=0x251176e0, sourceOffset=0, count=1090) at /work/mozilla/builds/2.0.0/mozilla/modules/libpr0n/src/imgLoader.cpp:2020
#3  0x04bc34d8 in nsStreamListenerTee::OnDataAvailable (this=0x25150520, request=0xd2fe34, context=0x0, input=0x23ca08ec, offset=0, count=1090) at /work/mozilla/builds/2.0.0/mozilla/netwerk/base/src/nsStreamListenerTee.cpp:111
#4  0x04c79499 in nsHttpChannel::OnDataAvailable (this=0xd2fe00, request=0x251e70a0, ctxt=0x0, input=0x23ca08ec, offset=0, count=1090) at /work/mozilla/builds/2.0.0/mozilla/netwerk/protocol/http/nsHttpChannel.cpp:4138
#5  0x04b8bad9 in nsInputStreamPump::OnStateTransfer (this=0x251e70a0) at /work/mozilla/builds/2.0.0/mozilla/netwerk/base/src/nsInputStreamPump.cpp:510
#6  0x04b8bff0 in nsInputStreamPump::OnInputStreamReady (this=0x251e70a0, stream=0x23ca08ec) at /work/mozilla/builds/2.0.0/mozilla/netwerk/base/src/nsInputStreamPump.cpp:400
#7  0x06271346 in nsInputStreamReadyEvent::Run (this=0x23cfbfd0) at /work/mozilla/builds/2.0.0/mozilla/xpcom/io/nsStreamUtils.cpp:114

Comment 1

[Daniel Holbert [:dholbert]]

Confirmed here: bp-d084ec8b-5f94-44f9-83de-c4bdc2110514
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110514 Firefox/6.0a1

mSVGDocumentWrapper is null when we crash.  Odd.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: testcase

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> mSVGDocumentWrapper is null when we crash.
...ah, it's null because we hit the failure case in VectorImage::OnStartRequest, and null it out (and toggle mError to true).

(We hit that failure case because the httpChannel tells us that "requestSucceeded" is false, in SVGDocumentWrapper::SetupViewer())

So the first thing we need here is an "if (mError)" early-return in OnDataAvailable.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix

Yup -- after adding an early return in VectorImage::OnDataAvailable and VectorImage::GetRootLayoutFrame, all is well.

(I made sure that those were the only two places in VectorImage.cpp where we deref mSVGDocumentWrapper without first checking mError.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 532496 [details] [diff] [review]
fix

Review of attachment 532496 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to add a test

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: test patch (mochitest w/ sjs file)

This patch has a mochitest that references a .sjs file to get an error 500 and trigger this bug.

For simplicity, I used "hg cp" to get boilerplate from another .sjs file, and then tweaked the copy to load an SVG image and also return HTTP status 500.

I've confirmed that this mochitest asserts & crashes without this bug's fix, and succeeds after I apply the fix.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 534345 [details] [diff] [review]
test patch (mochitest w/ sjs file)

Review of attachment 534345 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 8

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/projects/cedar/rev/95ff659eafdb
http://hg.mozilla.org/projects/cedar/rev/51242e6051ec

Comment 9

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/95ff659eafdb
http://hg.mozilla.org/mozilla-central/rev/51242e6051ec

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 532496 [details] [diff] [review]
fix

Requesting permission to land this on aurora & beta.  This patch just adds a error-flag-check early-return to 2 methods that were missing this check. (see comment 4).

* Reward: Fixes crash that was introduced in Firefox 4 --> improved stability
* Risk: Low. Safe, minimal fix.

Includes regression test.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 532496 [details] [diff] [review]
fix

Plus for aurora, but given how rare this is we'll hold off for beta.

Comment 12

[Daniel Holbert [:dholbert]]

Gah, sorry -- I'm failing today at remembering whether things landed before or after the last aurora merge.  (I thought the merge was longer ago than it actually was.)

The checkin in comment 9 beat the merge, so this is already in aurora.

Comment 13

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on WinXP, Ubuntu 11.04 x86, Win7 x86, Mac OS X 10.6 using the steps from comment 0.

Crash no longer reproducible -> setting status to Verified Fixed.