600020 - "ASSERTION: Must know the source we were loading from"

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: stack traces

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for the testcase Jesse!

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 (Not reading bugmail)]]

(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]

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 (Not reading bugmail)]]

(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 (Not reading bugmail)]]

Attachment: 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) (email my personal email if necessary)]

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 (Not reading bugmail)]]

(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 (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/eb15fab87e7e

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.