1124844 - When video is loaded from VideoDocument, NotifyOwnerDocumentActivityChanged() is not called

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #1113527 +++

This bug is created based on Bug 1113527 Comment 65 and Bug 1113527 Comment 66.

When video is loaded from VideoDocument, HTMLVideoElement::NotifyOwnerDocumentActivityChanged() is not called, then MediaDecoder::NotifyOwnerActivityChanged() is also not called. when the document becomes visible.

Comment 1

[Sotaro Ikeda [:sotaro]]

The following is a copy of Bug 1113527 Comment 66. 

-----------------------------------------------------------------

From the investigation, the following things becomes clear.
- VideoDocument creates HTMLMediaElement during VideoDocument's load.
  VideoDocument::CreateSyntheticVideoDocument() is called in VideoDocument::StartDocumentLoad()
  https://dxr.mozilla.org/mozilla-central/source/dom/html/VideoDocument.cpp#84

- When document's visibility state is updated by nsDocument::SetScriptGlobalObject(), HTMLVideoElement::NotifyOwnerDocumentActivityChanged() is not called.
  From the following, it could happen at "coming out of bfcache" and "initial document load".
  https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#4711

Comment 2

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> 
> - When document's visibility state is updated by
> nsDocument::SetScriptGlobalObject(),
> HTMLVideoElement::NotifyOwnerDocumentActivityChanged() is not called.
>   From the following, it could happen at "coming out of bfcache" and
> "initial document load".
>   https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#4711

It seems necessary to add calling EnumerateActivityObservers(NotifyActivityChanged, nullptr) without firing the event.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

Comment 4

[Sotaro Ikeda [:sotaro]]

I confirmed that attachment 8553340 [details] [diff] [review] fixed the problem of Bug 1113527 Comment 55 on master flame.

Comment 5

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8553340 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

cpearce, can I have a feedback to the patch?

Comment 6

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

Comment on attachment 8553340 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

Review of attachment 8553340 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me, but you should get feedback/review from bz or another DOM peer. This will also notify <object> tags for example, which may have consequences that I don't understand.

Comment 7

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8553340 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

bz, can you review the patch? Thanks!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553340 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

I guess, though it would have been better to pick on whoever added this code or reviewed the addition, no?

However, you really need to add comments explaining why the reasoning about not firing the event doesn't apply to the activity change notification.

r=me with that.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject() (r=bz)

Apply the comment. Carry "r=bz".

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 8553340 [details] [diff] [review]
> patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()
> 
> I guess, though it would have been better to pick on whoever added this code
> or reviewed the addition, no?

visibility code is added by you at Bug 690056. You seems correct person to review this bug.

> 
> However, you really need to add comments explaining why the reasoning about
> not firing the event doesn't apply to the activity change notification.

I added the comment.

> 
> r=me with that.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> visibility code is added by you at Bug 690056.

Yes, but the notification code isn't, and what you're asking is whether calling _that_ is correct.

Put another way: I have approximately 0 confidence in my ability to have caught an issue in this patch, and I do not know the answers to the questions comment 6 raises.

Comment 12

[Boris Zbarsky [:bzbarsky]]

And your comment doesn't address my review comment, which was about explaining why the reasoning for not firing the event doesn't apply.  That part needs to be made crystal clear: why do we need to notify if we're still in the initial setup of the document?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553864 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject() (r=bz)

Given that, r- until I see a version of this comment that addresses my review comments.

Comment 14

[Sotaro Ikeda [:sotaro]]

When video is loaded via VideoDocument, HTMLMediaElement and MediaDecoder creation are already done before nsDocument::SetScriptGlobalObject() call. MediaDecoder decide whether starting video decoding is decided based on document's visibility. When the MediaDecoder is created, nsDocument::SetScriptGlobalObject() is not yet called and document is hidden state. Therefore the MediaDecoder decides that video decoding is not yet necessary. But soon after nsDocument::SetScriptGlobalObject() is called and document becomes not hidden. At the time, MediaDecoder need to know document becomes not hidden state, and need to start updating video decoding. But nsDocument::SetScriptGlobalObject() does not call NotifyActivityChanged(), therefore, MediaDecoder can not know document visibility is changed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, there you go.  _That_ should go in the code comment.

Comment 16

[Sotaro Ikeda [:sotaro]]

Thanks! I'll update the patch.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

Update the comment.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553903 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

Thank you!  I'm much happier about this change now.

Comment 19

[Sotaro Ikeda [:sotaro]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/bfde34168702

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bfde34168702

Comment 21

[Ralph Giles (:rillian)]

bz, could you comment on the risk of uplifting this to beta/aurora? I'd like it for the MSE feature, but don't know enough to characterize the risk for other dom areas.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'd be pretty comfortable with uplifting this to Aurora.

As for beta, I don't know.  I get pretty worried whenever we change this code; it's very finicky.  I _think_ it would be ok, but it worries me.

Comment 23

[Ralph Giles (:rillian)]

Ok. Thanks. I didn't help with the beta issue I saw, so likely we can skip it for 36.

Comment 24

[Ralph Giles (:rillian)]

Comment on attachment 8553903 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, Youtube playback can use more resources.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Risk seems reasonably low. We've had this in nightly for a week.
[String/UUID change made/needed]: None.

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8553903 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

Taking this on Aurora based on bz's risk assessment in comment 22. Aurora+

Comment 26

[Ralph Giles (:rillian)]

Thanks. https://hg.mozilla.org/releases/mozilla-aurora/rev/d01793855eba