479711 - Media elements should delay the document 'load' event

Status: RESOLVED FIXED

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

Description

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

See http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-February/018624.html

We should do this because it will make media elements easier to use, it integrates media elements into HTML in a useful way that plugins can't, and it will let us simplify our reftests.

Boris, what's the best way to do this? If we put media loads in the document loadgroup, is there a way to take them out when the decoder reaches the HAVE_CURRENT_DATA state? Or do we need to put a synthetic request into the loadgroup so that we can pretend it's finished when we reach HAVE_CURRENT_DATA?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Moving loads in and out of the loadgroups sounds error-prone to me; I assume that right now those are background loads, right?

It might be that the right answer here is to just make nsIDocument::BlockOnload and UnblockOnload calls at the right times.

Comment 2

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

Right now these loads aren't associated with any loadgroup.

BlockOnLoad/UnblockOnLoad sounds fine, we can use that.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Right now these loads aren't associated with any loadgroup.

That's really bad, actually.  What stops them when the user closes the tab or navigates to a different page or some such?  Or heck hits the Stop button?

Comment 4

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

nsHTMLMediaElement::DestroyContent and PresShell::Freeze. Stop button doesn't do anything.

Comment 5

[Boris Zbarsky [:bzbarsky]]

And we make sure that we don't ever start those loads for nodes that are in disconnected fragments?  That works, I guess.  Is not exposing various info about the media file through the DOM in that situation ok?

In any case, I guess that's all not that relevant to this bug.

Comment 6

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

(In reply to comment #5)
> And we make sure that we don't ever start those loads for nodes that are in
> disconnected fragments?

No, we can start loads for disconnected elements, as per spec. The fact that PresShell::Freeze doesn't find them is a filed bug I think. Well, it's known anyway.

Hmm, I didn't think about whether a load in a disconnected media element should delay the load event in a document. I guess it shouldn't.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Putting that into PresShell::Freeze doesn't make sense; the document might not even have a presshell...

It seems to me that loads should go in the document loadgroup, unless you coalesce them across documents, with the LOAD_BACKGROUND flag if you don't want them affecting the throbber or directly affecting onload.  Then all the load-canceling stuff will Just Work for free; that's the point of loadgroups.  You'll still need to manually manage onload, of course.

Comment 8

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

OK. However, we still need to do something to stop the media playing when the document goes into the bfcache, even if the load is automatically suspended. So we still have a bug there.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Documents with active loads don't go into bfcache.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Active as far as the loadgroup goes, that is.

Comment 11

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

That's not good. Media elements often keep loading forever, but we don't want such pages to not be bf-cachable.

Comment 12

[Boris Zbarsky [:bzbarsky]]

We took that hit for other pages with loading forever elements (e.g. images that are using JPEG push), precisely because we had no good solution for stopping and restarting the network loads as needed....

Maybe the media situation is better because you can always use byte-range requests or the like to restart?

Comment 13

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

Is now a good time to ask what nsIRequest::Suspend/Resume actually does?

Comment 14

[Boris Zbarsky [:bzbarsky]]

For a lot of requests, "not much".

For HTTP, I _think_ it just makes us not read data off the socket, which works great until the server times us out.  But biesi or darin would know better...

Comment 15

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

OK, then I think using those for bf-cached pages is reasonable for us; that's what we're currently doing.

We should probably detect a dropped connection somehow and restart it as necessary, at least if the server supports byte-range requests.

Thanks. I think we should probably proceed with not having our requests in the loadgroup, and have an alternative solution for freezing, based on tracking a list of all playing elements for a given document.

Comment 16

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

(and we should use the same approach for freezing plugins too really, since display:none plugins really should be supported)

Comment 17

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

I mean, plugins in display:none IFRAMEs

Comment 18

[Boris Zbarsky [:bzbarsky]]

Alternately, we should have a way to flag requests that bfcache should not bail on but should instead suspend/resume.  Or something.  Centralize that logic and such...

Comment 19

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

Attachment: Patch v1

Adds delaying-the-load-event flag to media element. Delays document load event until metadataloaded, or load error.

We set the flag to true (and so delay the doc's load event) in:
* QueueSelectResourceTask()
* QueueLoadFromSourceTask()
* LoadWithChannel()

This is actually stretching the spec a bit. The spec says we call the media-selection algorithm asynchronously, and we don't set the flag until we're in the media-selection algorithm. But if we do that, the load can finish before the async-call event fires, so we need to set the flag before we start the async call. I've posted to WHATWG about this, I'm still awaiting reply:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-March/018679.html

The delaying-the-load-event flag is set to false as per the spec:
* After loadedmetadata.
* When we wait for src/source to be added.
* On error.

I also added a test, and removed the reftest-wait for ogg reftests.

Comment 20

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

(In reply to comment #19)
> The delaying-the-load-event flag is set to false as per the spec:
> * After loadedmetadata.

This should be after FirstFrameLoaded. The spec doesn't currently say that but Hixie agreed, I think:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-February/018663.html

One question is whether an element that is not in the document should delay the load event. I kinda think it shouldn't. The spec suggests it should, but isn't explicit about it. What do you think?

Comment 21

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

(In reply to comment #20)
> One question is whether an element that is not in the document should delay the
> load event. I kinda think it shouldn't. The spec suggests it should, but isn't
> explicit about it. What do you think?

I think media not in a document should not delay the load event. The purpose of delaying the load event was primarily to allow layout to stabilize before firing the load event, and things not in the document won't affect layout... If they're added to the document before load fires, and they start to load, the load event will be delayed anyway, and if they aren't added to the doc, they won't affect the initial reflow.

Do we delay the load event for images outside of the document?

Hmm... I wonder if load still fires if we remove a loading media during load...

Comment 22

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

(In reply to comment #21)
> Hmm... I wonder if load still fires if we remove a loading media during load...

It seems so, but only once the detached media loads enough to set the delaying-the-load-event to false. If we're don't delay the load event for things not in the document, then I should probably set the delaying-the-load-event flag to false in UnbindFromTree() as well.

Comment 23

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

(In reply to comment #21)
> (In reply to comment #20)
> > One question is whether an element that is not in the document should delay the
> > load event. I kinda think it shouldn't. The spec suggests it should, but isn't
> > explicit about it. What do you think?
> 
> I think media not in a document should not delay the load event. The purpose of
> delaying the load event was primarily to allow layout to stabilize before
> firing the load event, and things not in the document won't affect layout... If
> they're added to the document before load fires, and they start to load, the
> load event will be delayed anyway, and if they aren't added to the doc, they
> won't affect the initial reflow.

I agree. But the patch you have here does delay the load event for not-in-doc elements and needs to be updated, right?

> Do we delay the load event for images outside of the document?

I don't know.

> Hmm... I wonder if load still fires if we remove a loading media during load...

We should stop delaying the load event in UnbindFromTree, yes.

Comment 24

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

Attachment: Patch v2

As patch v1, except:
* Stops delaying load event in UnbindFromTree().
* Stops delaying load event in FirstFrameLoaded() instead of MetadataLoaded().
* ChangeDelayLoadStatus() bails before changing state if !IsInDoc().

Comment 25

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

+    is(gLoadedMetaData, true, "onload was not delayed until after metadataloaded");

This should be loadeddata now I guess...

You might also want a test to verify that if an element not in a document starts loading, but hasn't finished loading before we bind it to a loading document, it blocks onload until it finishes loading. And another test to verify that if an element in a document starts loading, but hasn't finished loading before we remove it from the document, it doesn't block onload. And another test to verify that if an element isn't delaying the load event, but we start a new load via various means (e.g. adding a <source>), then it does delay the load event if necessary.

(In reply to comment #24)
> * ChangeDelayLoadStatus() bails before changing state if !IsInDoc().

I think this isn't right. We should change our internal state always, we just don't want to change the document load blocking state when we're not in the document. We need something in BindToTree to start blocking the load event if our delay-the-load-event flag is set ... and UnbindFromTree shouldn't change our delay-the-load-event flag, it should just unblock the load event if our flag is set. Make sense?

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Do we delay the load event for images outside of the document?

Yes.  Typically, those are image preloads and pages depend on those being loaded before onload fires.

> Hmm... I wonder if load still fires if we remove a loading media during load...

Write a test, please.  And yes, write the tests comment 25 mentions.

Comment 27

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

Hmm. I think being consistent with images trumps other considerations. So let's make out-of-doc elements block their owner document's load event, after all.

Comment 28

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

But we'll have to be careful that when the owner document changes for an out-of-document media element, we transfer our block correctly. We'll need tests for that.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Or you could do what images do: don't change which document you're blocking on.  That is, when you block save the pointer to the document you blocked on, and make sure to unblock that same document.

Comment 30

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

OK then, let's do that.

Comment 31

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

Attachment: Patch v3

Similar to patch v1, but blocks loads on the document that was its owner when load started. Does not transfer blocking over to another document if the element is moved between documents. Also added guard to prevent multiple async events to call SelectResource() being set at the same time. This means that there can only ever be one execution of SelectResource() happening at once. Added some more tests too.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Might be worth dropping the pointer to the document after you unblock.

Comment 33

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

Comment on attachment 365805 [details] [diff] [review]
Patch v3

Yeah, null out mLoadBlockedDoc after unblocking.

Comment 34

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

Attachment: Patch v4 [Checkin: Comment 36]

As patch v3, with nulling out mLoadBlockedDoc. Still seem to need the cycle collection to traverse mLoadBlockedDoc though.

Comment 35

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

Bug 479859 should land before this.

Comment 36

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

http://hg.mozilla.org/mozilla-central/rev/37b4cf75a6a5

Comment 37

[Serge Gautherie (:sgautherie)]

checkin-needed: can't be landed as blocking bug misses updated patch.

Comment 38

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d9feaa4fbac7

Comment 39

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3970d56aac0b

Comment 40

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

Somehow I missed one hunk earlier.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dbeeaf6cb02d