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

RESOLVED FIXED in Firefox 37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 wontfix, firefox37+ fixed, firefox38 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
+++ 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.
(Assignee)

Updated

4 years ago
Blocks: 1123452
Priority: P5 → --
(Assignee)

Updated

4 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Updated

4 years ago
Blocks: 1050031
No longer blocks: 1123452
(Assignee)

Comment 1

4 years ago
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/﷒0﷓

- 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/﷒1
(Assignee)

Comment 2

4 years ago
(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/﷒0

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

Comment 3

4 years ago
Created attachment 8553340 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()
(Assignee)

Comment 4

4 years ago
I confirmed that attachment 8553340 [details] [diff] [review] fixed the problem of Bug 1113527 Comment 55 on master flame.
(Assignee)

Comment 5

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

cpearce, can I have a feedback to the patch?
Attachment #8553340 - Flags: feedback?(cpearce)
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.
Attachment #8553340 - Flags: feedback?(cpearce) → feedback+
(Assignee)

Comment 7

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

bz, can you review the patch? Thanks!
Attachment #8553340 - Flags: review?(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.
Attachment #8553340 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

4 years ago
Created attachment 8553864 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject() (r=bz)

Apply the comment. Carry "r=bz".
Attachment #8553340 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
(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.
> 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.
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 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.
Attachment #8553864 - Flags: review-
(Assignee)

Comment 14

4 years ago
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.
OK, there you go.  _That_ should go in the code comment.
(Assignee)

Comment 16

4 years ago
Thanks! I'll update the patch.
(Assignee)

Comment 17

4 years ago
Created attachment 8553903 [details] [diff] [review]
patch - Add NotifyActivityChanged call to nsDocument::SetScriptGlobalObject()

Update the comment.
Attachment #8553864 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8553903 - Flags: review?(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.
Attachment #8553903 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/bfde34168702
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Ok. Thanks. I didn't help with the beta issue I saw, so likely we can skip it for 36.
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.
Attachment #8553903 - Flags: approval-mozilla-aurora?
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+
Attachment #8553903 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox36: --- → wontfix
status-firefox37: --- → affected
status-firefox38: --- → fixed
tracking-firefox37: --- → +
OS: Gonk (Firefox OS) → All
(Assignee)

Updated

4 years ago
blocking-b2g: --- → 2.2?

Updated

4 years ago
blocking-b2g: 2.2? → ---
You need to log in before you can comment on or make changes to this bug.