If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Intermittent test_load_candidates.html | called finish() multiple times

RESOLVED FIXED in Firefox 35, Firefox OS v2.1

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Tomcat, Assigned: bechen)

Tracking

({intermittent-failure})

Trunk
mozilla37
intermittent-failure
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed, firefox37 fixed, firefox-esr31 unaffected, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
b2g_emulator_vm mozilla-inbound opt test mochitest-6

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3682997&repo=mozilla-inbound

01:13:08 INFO - 125 INFO TEST-UNEXPECTED-ERROR | /tests/dom/media/test/test_load_candidates.html | called finish() multiple times
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Hi JW, any ideas what might be going on here? :)
Flags: needinfo?(jwwang)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
http://dxr.mozilla.org/mozilla-central/source/dom/media/test/test_load_candidates.html
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
I will try to add some logs to see what's going on.
Assignee: nobody → jwwang
Flags: needinfo?(jwwang)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Ok, I see what happened:

1. v.parentNode.removeChild(v) [1] calls HTMLMediaElement::UnbindFromTree() and the decoder enters the dormant state.
2. somehow HTMLMediaElement::NotifyOwnerDocumentActivityChanged is called again with |ownerDoc->Hidden() == false| to let the decoder exit the dormant state.
3. the decoder starts to decode metadata again and fires loadeddata in the end.
4. v.parentNode.removeChild(v) is called again and we get JavaScript Error: "TypeError: v.parentNode is null".
5. SimpleTest.finish() is called again.

[1] http://hg.mozilla.org/mozilla-central/annotate/a52bf59965a0/dom/media/test/test_load_candidates.html#l32
[2] http://hg.mozilla.org/mozilla-central/annotate/a52bf59965a0/dom/media/test/test_load_candidates.html#l33
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Created attachment 8527431 [details] [diff] [review]
1095438_fix_test_load_candidates_html.patch

Don't leave dormant state while detached from the DOM tree.

See comment 38 for the root cause.

Try: https://tbpl.mozilla.org/?tree=Try&rev=fcb51f036ea6
Attachment #8527431 - Flags: review?(cpearce)
Component: DOM → Video/Audio
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Attachment #8527431 - Flags: review?(cpearce) → review+
Comment hidden (Treeherder Robot)
Keywords: checkin-needed
(Reporter)

Comment 78

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/feea8b1ea75f
Keywords: checkin-needed
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 81

3 years ago
sorry incorrectly blamed this checkin for causing a mulet test failure, re-checkedin in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=e04692266f17
Comment on attachment 8527431 [details] [diff] [review]
1095438_fix_test_load_candidates_html.patch

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

::: dom/html/HTMLMediaElement.cpp
@@ +3469,5 @@
>      mDecoder->SetElementVisibility(!ownerDoc->Hidden());
> +    // Since we enter dormant state in UnbindFromTree(),
> +    // we should only leave dormant state after binding to the tree.
> +    nsCOMPtr<nsIDOMNode> parent;
> +    nsresult rv = GetParentNode(getter_AddRefs(parent));

Why are we adding more nsIDOMNode usage?
Comment hidden (Treeherder Robot)
No, bug 1104703 wasn't a new intermittent. It was permafail from this. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/997d1d16fc19
Depends on: 1104703
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment on attachment 8527431 [details] [diff] [review]
1095438_fix_test_load_candidates_html.patch

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

::: dom/html/HTMLMediaElement.cpp
@@ +3469,5 @@
>      mDecoder->SetElementVisibility(!ownerDoc->Hidden());
> +    // Since we enter dormant state in UnbindFromTree(),
> +    // we should only leave dormant state after binding to the tree.
> +    nsCOMPtr<nsIDOMNode> parent;
> +    nsresult rv = GetParentNode(getter_AddRefs(parent));

Why don't you tell us what we should use instead?
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Created attachment 8529996 [details] [diff] [review]
1095438_part2_insert_media_elements_into_DOM_tree.patch

With patch part 1, media elements will go into dormant state while not binding to the DOM tree and therefore can't play normally.

Some test cases don't insert media elements into the DOM tree and therefore can't play to the end and result in test case timeouts.

Try again: https://tbpl.mozilla.org/?tree=Try&rev=c990905ab943
Attachment #8529996 - Flags: review?(cpearce)
(In reply to JW Wang [:jwwang] from comment #95)
> Created attachment 8529996 [details] [diff] [review]
> 1095438_part2_insert_media_elements_into_DOM_tree.patch
> 
> With patch part 1, media elements will go into dormant state while not
> binding to the DOM tree and therefore can't play normally.
> 
> Some test cases don't insert media elements into the DOM tree and therefore
> can't play to the end and result in test case timeouts.

Hmm... We should still be able to play media elements that aren't in the DOM... We should really fix that so it works on B2G, rather than working around it.

Can we move the video element out of dormant state if we try to play and it's not in the DOM but its OwnderDoc is still visible? Would that work?
It is kinda complicated. If we allow playing when not in the DOM, we then should enter dormant state once playback reaches the end or pause() is called. In other words, we have to check:

1. binding to DOM tree
2. document visibility
3. playing state

in order to decide whether to enter/exit dormant state.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Does the change made in this bug to make off-screen video elements dormant break the screen-shotting in the B2G Video App. That relies on an off-screen video element:

https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/metadata.js#L165
Flags: needinfo?(jwwang)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(In reply to Chris Pearce (:cpearce) from comment #107)
> Does the change made in this bug to make off-screen video elements dormant
> break the screen-shotting in the B2G Video App. That relies on an off-screen
> video element:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/metadata.
> js#L165

Yeah, I think so. And the rules in comment 97 is complicated. Here is another approach:
We should only take in/out tree status into account after a media element is detached from the tree.

1. When a media element is created and played off the tree. The dormant state is aligned with the hidden status of the owning document. This rule will make off-screen playback of video elements work.

2. When a media element is attached to the tree, the dormant state is aligned with the hidden status of the owning document. This rule conforms to the original design of entering/exiting dormant state.

3. After a media element is detached from the tree, the dormant state should consider both the hidden status of the owning document and in/out tree status. This rule will solve the problem of this bug and will not break rule 1 & 2.

Hi Benjamin,
Since this bug is related to your work in bug 1029552. Can you take this bug? Thanks.
Flags: needinfo?(jwwang) → needinfo?(bechen)
Comment on attachment 8529996 [details] [diff] [review]
1095438_part2_insert_media_elements_into_DOM_tree.patch

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

Will wait for fix from bechen.
Attachment #8529996 - Flags: review?(cpearce)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Updated

3 years ago
Assignee: jwwang → bechen
Flags: needinfo?(bechen)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 136

3 years ago
Created attachment 8534145 [details] [diff] [review]
bug-1095438.patch
Attachment #8534145 - Flags: review?(jwwang)
(Assignee)

Updated

3 years ago
Attachment #8534145 - Attachment is obsolete: true
Attachment #8534145 - Flags: review?(jwwang)
(Assignee)

Comment 137

3 years ago
Created attachment 8534147 [details] [diff] [review]
bug-1095438.patch
Attachment #8534147 - Flags: review?(jwwang)
Comment on attachment 8534147 [details] [diff] [review]
bug-1095438.patch

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

Overall looks fine.

::: dom/html/HTMLMediaElement.cpp
@@ +3483,5 @@
>      mDecoder->SetElementVisibility(!ownerDoc->Hidden());
> +    if (mHadBindToTree) {
> +      // Check if we are in tree or not.
> +      nsCOMPtr<nsIDOMNode> parent;
> +      nsresult rv = GetParentNode(getter_AddRefs(parent));

See comment 82, it looks like the usage of nsIDOMNode is discouraged. We might add another flag to remember whether this element is in tree or not. And this flag can be combined with mHadBindToTree for we only take in/out tree status per rule 3 of comment 119.

However, we should ask Ms2ger about nsIDOMNode usage to know the correct way to query in/out tree status.
Attachment #8534147 - Flags: review?(jwwang) → review+
Comment hidden (Treeherder Robot)
The replacement for nsIDOMNode is nsINode; however, the GetParentNode() check appears to be incorrect. A node that isn't in the document can still have a parent node.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Should we use nsINode::IsInDoc()?
Flags: needinfo?(Ms2ger)
  /**
   * @deprecated
   */
  bool IsInDoc() const

Perhaps IsInUncomposedDoc or IsInComposedDoc.
Flags: needinfo?(Ms2ger)
Should we even care about a media element that is not in the document? It should've  gone into the dormant state before losing its document, right?
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 149

3 years ago
(In reply to JW Wang [:jwwang] from comment #145)
> Should we even care about a media element that is not in the document? It
> should've  gone into the dormant state before losing its document, right?

I think we can open another bug to introduce the |IsInUncomposedDoc or IsInComposedDoc| into dormant state.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 158

3 years ago
Created attachment 8536440 [details] [diff] [review]
bug-1095438.patch

r=jwwang
tryserver:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=829c48839a28
Attachment #8527431 - Attachment is obsolete: true
Attachment #8529996 - Attachment is obsolete: true
Attachment #8534147 - Attachment is obsolete: true
Attachment #8536440 - Flags: review+
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 171

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c6f41906a9
Keywords: checkin-needed
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 174

3 years ago
https://hg.mozilla.org/mozilla-central/rev/53c6f41906a9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Please request Aurora approval for this patch when you get a chance. Also, does the underlying problem affect older Gecko revisions as well?
status-b2g-v2.1: --- → ?
status-b2g-v2.2: --- → fixed
status-firefox35: --- → ?
status-firefox36: --- → affected
status-firefox37: --- → fixed
status-firefox-esr31: --- → unaffected
Flags: needinfo?(bechen)
(Assignee)

Comment 179

3 years ago
Comment on attachment 8536440 [details] [diff] [review]
bug-1095438.patch

The patch can clean apply to aurora.

Because it is a regression by bug 1029552, so it affect gecko version later than mozilla 35.

Approval Request Comment
[Feature/regressing bug #]: bug 1029552
[User impact if declined]: Testcase test_load_candidates.html fail and the dormant status of a MediaElement which is not in the DOM tree may be wrong.
[Describe test coverage new/current, TBPL]: current. test_load_candidates.html.
[Risks and why]: low.
[String/UUID change made/needed]: none
Flags: needinfo?(bechen)
Attachment #8536440 - Flags: approval-mozilla-aurora?
Attachment #8536440 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-b2g-v2.1: ? → unaffected
status-firefox35: ? → affected
Comment on attachment 8536440 [details] [diff] [review]
bug-1095438.patch

See comment 179.
Attachment #8536440 - Flags: approval-mozilla-beta?
Attachment #8536440 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/806aebe9eed4
https://hg.mozilla.org/releases/mozilla-beta/rev/87470bb03b6c
status-firefox35: affected → fixed
status-firefox36: affected → fixed
(Assignee)

Comment 182

3 years ago
Created attachment 8538932 [details] [diff] [review]
bug-1095438.b2g34v2_1.patch

I just found that the bug 1092522 had exist in b2g34_v2_1. So I made v2.1 patch need to uplift to v2.1.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bgu 1029552
User impact if declined: Testcase test_load_candidates.html fail and the dormant status of a MediaElement which is not in the DOM tree may be wrong.
Testing completed: test_load_candidates.html on mozilla-central.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #8538932 - Flags: approval-mozilla-b2g34?
Flags: qe-verify-

Updated

3 years ago
Attachment #8538932 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
status-b2g-v2.1: unaffected → affected
Sorry, this approval got missed due to the v2.1 status flag being set to unaffected.

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/50b326d1ec0f
status-b2g-v2.1: affected → fixed

Comment 184

3 years ago
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/50b326d1ec0f
status-b2g-v2.1S: --- → fixed
You need to log in before you can comment on or make changes to this bug.