Closed
Bug 1095438
Opened 10 years ago
Closed 10 years ago
Intermittent test_load_candidates.html | called finish() multiple times
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: cbook, Assigned: bechen)
References
()
Details
(Keywords: intermittent-failure)
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
bechen
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bajaj
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 25•10 years ago
|
||
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 28•10 years ago
|
||
I will try to add some logs to see what's going on.
Updated•10 years ago
|
Assignee: nobody → jwwang
Flags: needinfo?(jwwang)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 38•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 71•10 years ago
|
||
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)
Updated•10 years ago
|
Component: DOM → Video/Audio
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•10 years ago
|
Attachment #8527431 -
Flags: review?(cpearce) → review+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 78•10 years ago
|
||
Keywords: checkin-needed
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Comment 81•10 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 82•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment 84•10 years ago
|
||
No, bug 1104703 wasn't a new intermittent. It was permafail from this. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/997d1d16fc19
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 90•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 95•10 years ago
|
||
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)
Comment 96•10 years ago
|
||
(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?
Comment 97•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 107•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 119•10 years ago
|
||
(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 120•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Updated•10 years ago
|
Assignee: jwwang → bechen
Flags: needinfo?(bechen)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 136•10 years ago
|
||
Attachment #8534145 -
Flags: review?(jwwang)
Assignee | ||
Updated•10 years ago
|
Attachment #8534145 -
Attachment is obsolete: true
Attachment #8534145 -
Flags: review?(jwwang)
Assignee | ||
Comment 137•10 years ago
|
||
Attachment #8534147 -
Flags: review?(jwwang)
Comment 138•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment 140•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 144•10 years ago
|
||
/**
* @deprecated
*/
bool IsInDoc() const
Perhaps IsInUncomposedDoc or IsInComposedDoc.
Flags: needinfo?(Ms2ger)
Comment 145•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 149•10 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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 158•10 years ago
|
||
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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Comment 171•10 years ago
|
||
Keywords: checkin-needed
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Comment 174•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 178•10 years ago
|
||
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•10 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?
Updated•10 years ago
|
Attachment #8536440 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Comment 180•10 years ago
|
||
Attachment #8536440 -
Flags: approval-mozilla-beta?
Updated•10 years ago
|
Attachment #8536440 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 181•10 years ago
|
||
Assignee | ||
Comment 182•10 years ago
|
||
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?
Updated•10 years ago
|
Flags: qe-verify-
Updated•10 years ago
|
Attachment #8538932 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Updated•10 years ago
|
Comment 183•10 years ago
|
||
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
Comment 184•10 years ago
|
||
status-b2g-v2.1S:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•