Closed Bug 489830 Opened 11 years ago Closed 11 years ago

Crash on air mozilla with firebug 1.4a14 [@ nsIndexedToHTML::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int)]

Categories

(Core :: Audio/Video, defect, critical)

x86
Windows Vista
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.1

People

(Reporter: tdowner, Assigned: kinetik)

References

Details

(Keywords: crash, verified1.9.1, Whiteboard: Firebug)

Crash Data

Attachments

(1 file, 1 obsolete file)

Go to air mozilla, crash.

bp-c20dfd67-97ce-4c16-920b-de6f42090423
OK, firebug causes this. should we be crashing though?
Summary: Crash on air mozilla, [nsIndexedToHTML::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int)] → Crash on air mozilla with firebug 1.4a14 [nsIndexedToHTML::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int)]
Whiteboard: Firebug
248 NS_ABORT_IF_FALSE(mNextListener, "Must have a listener");
249 return mNextListener->OnDataAvailable(aRequest, aContext, aStream, aOffset, aCount); 

so. I claim that NS_ABORT_IF_FALSE is bogus. kinetik: just deal.
Assignee: nobody → kinetik
Component: General → Video/Audio
Depends on: 464158
Product: Firefox → Core
QA Contact: general → video.audio
Summary: Crash on air mozilla with firebug 1.4a14 [nsIndexedToHTML::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int)] → Crash on air mozilla with firebug 1.4a14 [@ nsIndexedToHTML::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int)]
It looks like Firebug's channel listener is breaking nsIRequest's contract.

nsHTMLMediaElement::MediaLoadListener::OnStartRequest is returning a failure nsresult which is supposed to cancel the request.  nsIRequest.idl's documentation for cancel says: "Requests that use nsIStreamListener must not call onDataAvailable anymore after cancel has been called."

The call to the next listener's OnStartRequest (in firebug-channel-listener.js) is wrapped in a try/catch, swallowing any exceptions and thus causing the request not to be canceled.

So, we try to cancel the request and Firebug's listener stops that from working.  Eventually our OnDataAvailable fires and we hit the assert.
matthew: it doesn't matter. you can't enforce such contracts against scriptable interfaces. the contract could be violated because of OOM or a Debugger (venkman, firebug, chromebug, ...) or some other legal behavior.

If what firebug is doing is wrong, you can certainly file a bug asking them to fix it, however you still need to fix your code.
Attached patch patch v0 (obsolete) — Splinter Review
Make the check for mNextListener a non-fatal assertion.  Now you'll get two non-fatal errors logged:

###!!! ASSERTION: Must have a chained listener; OnStartRequest should have canceled this request: 'Error', file /Users/kinetik/work/mozilla-central/mozilla/content/html/content/src/nsHTMLMediaElement.cpp, line 249
###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error', file /Users/kinetik/work/mozilla-central/mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 529

I've filed a bug against Firebug 1.4 about the behaviour of their channel listener.  It's issue 1712: http://code.google.com/p/fbug/issues/detail?id=1712
Attachment #375369 - Flags: superreview?(roc)
Attachment #375369 - Flags: review?(roc)
Attachment #375369 - Flags: superreview?(roc)
Attachment #375369 - Flags: superreview+
Attachment #375369 - Flags: review?(roc)
Attachment #375369 - Flags: review+
Comment on attachment 375369 [details] [diff] [review]
patch v0

Looks OK, but take out all the code that wasn't meant to be part of this patch!
Attached patch patch v1Splinter Review
Thanks, I forgot to restrict the diff.
Attachment #375369 - Attachment is obsolete: true
Attachment #375384 - Flags: superreview+
Attachment #375384 - Flags: review+
Keywords: checkin-needed
Whiteboard: Firebug → Firebug, [needs landing]
We probably want this on 1.9.1 too.
Flags: wanted1.9.1?
Pushed http://hg.mozilla.org/mozilla-central/rev/26e323fea008
Keywords: checkin-needed
Whiteboard: Firebug, [needs landing] → Firebug
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: wanted1.9.1? → wanted1.9.1+
Whiteboard: Firebug → Firebug, [needs 191 landing]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/185f88b97baa
Keywords: fixed1.9.1
Whiteboard: Firebug, [needs 191 landing] → Firebug
Target Milestone: --- → mozilla1.9.1
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090511 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090511042228
V. Thanks
Status: RESOLVED → VERIFIED
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090526 Shiretoko/3.5pre Ubiquity/0.1.5.  Running Firebug 1.4.0a19, no crash.
Crash Signature: [@ nsIndexedToHTML::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int)]
You need to log in before you can comment on or make changes to this bug.