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)

Product:
Core
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37
Tracking Flags:
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed
firefox-esr31 --- unaffected
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed

People

(Reporter: cbook, Assigned: bechen)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 4 obsolete files)

bug-1095438.patch
4.08 KB, patch
bechen
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
bug-1095438.b2g34v2_1.patch
4.26 KB, 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
Hi JW, any ideas what might be going on here? :)
Flags: needinfo?(jwwang)
I will try to add some logs to see what's going on.
Assignee: nobody → jwwang
Flags: needinfo?(jwwang)
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
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
Attachment #8527431 - Flags: review?(cpearce) → review+
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?
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 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?
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.
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)
(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)
Assignee: jwwang → bechen
Flags: needinfo?(bechen)
Attached patch bug-1095438.patch (obsolete) — — Splinter Review 

        Attachment #8534145 -
        Flags: review?(jwwang)

    
  

        Attachment #8534145 -
        Attachment is obsolete: true

        Attachment #8534145 -
        Flags: review?(jwwang)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug-1095438.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        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+

    
    

    
      
  

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  
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+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Keywords: checkin-needed

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/53c6f41906a9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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)

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  


  
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-

        Attachment #8538932 -
        Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+

    
    

    
  
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

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: