Last Comment Bug 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'
: When SVG background hits HTTP 500: crash [@ mozilla::imagelib::VectorImage::O...
Status: VERIFIED FIXED
: assertion, crash, reproducible, testcase
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla6
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
http://maggie.idium.no/ntg.no/
Depends on:
Blocks: 532972
  Show dependency treegraph
 
Reported: 2011-05-14 21:09 PDT by Bob Clary [:bc:]
Modified: 2011-07-28 07:00 PDT (History)
4 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (155 bytes, text/html)
2011-05-14 22:11 PDT, Bob Clary [:bc:]
no flags Details
fix (1.52 KB, patch)
2011-05-14 22:29 PDT, Daniel Holbert [:dholbert]
roc: review+
jst: approval‑mozilla‑beta-
Details | Diff | Review
test patch (mochitest w/ sjs file) (3.64 KB, patch)
2011-05-22 19:15 PDT, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Review

Description Bob Clary [:bc:] 2011-05-14 21:09:10 PDT
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] 2011-05-14 22:07:45 PDT
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:] 2011-05-14 22:11:58 PDT
Created attachment 532494 [details]
testcase
Comment 3 Daniel Holbert [:dholbert] 2011-05-14 22:20:22 PDT
(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] 2011-05-14 22:29:47 PDT
Created attachment 532496 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-05-15 16:30:53 PDT
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] 2011-05-22 19:15:50 PDT
Created attachment 534345 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-05-22 19:20:22 PDT
Comment on attachment 534345 [details] [diff] [review]
test patch (mochitest w/ sjs file)

Review of attachment 534345 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 10 Daniel Holbert [:dholbert] 2011-05-31 17:26:42 PDT
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, jst@mozilla.com) 2011-06-01 11:47:59 PDT
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] 2011-06-01 12:26:55 PDT
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 2011-07-28 07:00:51 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.