Last Comment Bug 600020 - "ASSERTION: Must know the source we were loading from"
: "ASSERTION: Must know the source we were loading from"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla6
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks: 343943 485288
  Show dependency treegraph
 
Reported: 2010-09-27 14:31 PDT by Jesse Ruderman
Modified: 2011-05-03 20:48 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (397 bytes, text/html)
2010-09-27 14:31 PDT, Jesse Ruderman
no flags Details
stack traces (2.62 KB, text/plain)
2010-09-27 14:33 PDT, Jesse Ruderman
no flags Details
Patch v1 (2.05 KB, patch)
2011-05-03 19:18 PDT, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Review

Description Jesse Ruderman 2010-09-27 14:31:45 PDT
Created attachment 478862 [details]
testcase

This is a variant on bug 593305 that triggers an additional assertion.  (The second assertion is the "new" one.)

###!!! ASSERTION: Load event should not be delayed at start of resource selection.: '!mDelayingLoadEvent', file content/html/content/src/nsHTMLMediaElement.cpp, line 632

###!!! ASSERTION: Must know the source we were loading from!: 'mSourceLoadCandidate', file content/html/content/src/nsHTMLMediaElement.cpp, line 688
Comment 1 Jesse Ruderman 2010-09-27 14:33:04 PDT
Created attachment 478864 [details]
stack traces
Comment 2 Chris Pearce (:cpearce) 2010-09-27 14:51:23 PDT
Thanks for the testcase Jesse!
Comment 3 Chris Pearce (:cpearce) 2010-10-25 20:40:51 PDT
The assertion "Must know the source we were loading from!" no longer occurs thanks to the landing of bug 604067. The "Must know the source we were loading from" assertion still occurs.
Comment 4 Chris Pearce (:cpearce) 2010-10-25 20:48:49 PDT
(In reply to comment #3)
> The assertion "Must know the source we were loading from!" no longer occurs
> thanks to the landing of bug 604067. The "Must know the source we were loading
> from" assertion still occurs.

Oops, I mean the assertion "Load event should not be delayed at start of resource
selection." still occurs.
Comment 5 Jesse Ruderman 2011-04-25 22:45:47 PDT
Now I get this assertion instead, which seems like it's kind of the opposite:

###!!! ASSERTION: Load event not delayed during source selection?: 'mDelayingLoadEvent', file content/html/content/src/nsHTMLMediaElement.cpp, line 527
Comment 6 Chris Pearce (:cpearce) 2011-05-03 17:37:30 PDT
(In reply to comment #5)
> Now I get this assertion instead, which seems like it's kind of the opposite:
> 
> ###!!! ASSERTION: Load event not delayed during source selection?:
> 'mDelayingLoadEvent', file content/html/content/src/nsHTMLMediaElement.cpp,
> line 527

This occurs because we're receiving an OnStartRequest for a channel which has been cancelled because we've started a new load. We're dispatching an error event in this case to the element (which is where we're triggering the assertion), even though the element has started a new load, and isn't necessarily in an error state. We should not dispatch an error event in the OnStartRequest for a channel which is no longer the currently loading channel; we'll have dispatched "emptied" and "abort" events when the new load aborts/cancels the old load.
Comment 7 Chris Pearce (:cpearce) 2011-05-03 19:18:58 PDT
Created attachment 529913 [details] [diff] [review]
Patch v1

* Abort in an nsMediaLoadListener's OnStartRequest() (without firing an error event) if the load has been cancelled.

Patch is greenish on Try
http://tbpl.mozilla.org/?tree=Try&rev=e99e885e06a4
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-03 19:23:40 PDT
Comment on attachment 529913 [details] [diff] [review]
Patch v1

Review of attachment 529913 [details] [diff] [review]:

Can we check in a test based on Jesse's test?
Comment 9 Chris Pearce (:cpearce) 2011-05-03 19:36:41 PDT
(In reply to comment #8)
> Can we check in a test based on Jesse's test?

No need really. This assertion was randomly failing with my patch for bug 652751 applied in content/media/test/crashtests/459431-1.html, so that effectively is a testcase.
Comment 10 Chris Pearce (:cpearce) 2011-05-03 20:04:01 PDT
http://hg.mozilla.org/mozilla-central/rev/eb15fab87e7e
Comment 11 Chris Pearce (:cpearce) 2011-05-03 20:48:18 PDT
(In reply to comment #9)
> This assertion was randomly failing with my patch for bug 652751 applied 

Oops, I meant with my patch for bug 650994 applied.

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