Closed Bug 1124844 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 + fixed
firefox38 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 2 obsolete files)

+++ 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.
Blocks: 1123452
Priority: P5 → --
Assignee: nobody → sotaro.ikeda.g
Blocks: 1050031
No longer blocks: 1123452
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
(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.
I confirmed that attachment 8553340 [details] [diff] [review] fixed the problem of Bug 1113527 Comment 55 on master flame.
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+
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+
Apply the comment. Carry "r=bz".
Attachment #8553340 - Attachment is obsolete: true
(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-
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.
Thanks! I'll update the patch.
Update the comment.
Attachment #8553864 - Attachment is obsolete: true
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
Closed: 5 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+
blocking-b2g: --- → 2.2?
blocking-b2g: 2.2? → ---
You need to log in before you can comment on or make changes to this bug.