Closed Bug 1239899 Opened 9 years ago Closed 8 years ago

High CPU usage when replacing video element using innerHTML clones hidden video element

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: cpearce, Assigned: jwwang)

References

(
URL
)

Details

(Keywords: crash, power, testcase, Whiteboard: [parity-IE] [parity-Chrome][parity-Edge])

Attachments

(5 files)

Testcase
(deleted), application/zip
Details
MozReview Request: Bug 1239899 - Pause media elements removed from document via calling pause(). r?roc
(deleted), text/x-review-board-request
roc
: review+
Details
Bug 1239899. Part 1 - Remove ElementInTreeState for ELEMENT_NOT_INTREE and ELEMENT_INTREE are not used at all. We can use a bool flag to do the job.
(deleted), text/x-review-board-request
cpearce
: review+
Details
Bug 1239899. Part 2 - per spec. await a stable state to pause the element when it is removed from the document.
(deleted), text/x-review-board-request
cpearce
: review+
Details
Bug 1239899. Part 3 - enable tests.
(deleted), text/x-review-board-request
cpearce
: review+
Details
Attached file Testcase (deleted) — 
Split off from Bug 1213344, the attached testcase causes high CPU usage:

Testcase: attachment 8673387 [details] 

<!DOCTYPE html>
<html>
<head>
</head>
<body>
  <video autoplay="autoplay" controls="" loop="loop" preload="none" height="350" width="610">
    <source src="Ironside_Malphite_Comic.webm" type="video/webm">
    <source src="Ironside_Malphite_Comic.mp4" type="video/mp4">
    <source src="Ironside_Malphite_Comic.ogv" type="video/ogv">
  </video>
<script>
(function(){
  var container = document.querySelector('body');
  var txt;

  for (var i = 0; i < 20; i++) {
    txt = container.innerHTML;
    container.innerHTML = txt;
  }

})();
</script>
</body>
</html>


I'm splitting this off from bug 123344 because that bug went on a wild goose chase after a different bug that happened to repro with the above testcase.
Keywords: crash, power, testcase
Whiteboard: [parity-IE] [parity-Chrome][parity-Edge]
My analysis from bug 1213344 comment #29:

> 
> (In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #19)
> > Should removing a <video> from the DOM pause it or anything like that?
> 
> Yes, removing a <video> from the DOM should cause it to pause. This is the
> problem we should address in this bug.
> 
> The w3 spec[1] says:
> 
> <quote>
> When a media element is removed from a Document, the user agent must run the
> following steps:
> 
> 1. Asynchronously await a stable state, allowing the task that removed the
> media element from the Document to continue. The synchronous section
> consists of all the remaining steps of this algorithm. (Steps in the
> synchronous section are marked with ⌛.)
> 
> 2. ⌛ If the media element is in a Document, abort these steps.
> 
> 3. ⌛ If the media element's networkState attribute has the value
> NETWORK_EMPTY, abort these steps.
> 
> 4. ⌛ Pause the media element.
> </quote>
> 
> We're supposed to pause playback in HTMLMediaElement::UnbindFromTree(). Not
> sure why that's not being hit here.
> 
> [1] http://dev.w3.org/html5/spec-preview/media-elements.html
MozReview request in bug 1200514.

https://reviewboard.mozilla.org/r/31493/

MozReview doesn't handle sets of changesets with different bug numbers gracefully, so the patch for this bug ended up in the review queue of the bug mentioned earlier in the set of changesets in the review push. :(
(In reply to Chris Pearce (:cpearce) from comment #4)
> MozReview request in bug 1200514.
> 
> https://reviewboard.mozilla.org/r/31493/
> 
> MozReview doesn't handle sets of changesets with different bug numbers
> gracefully, so the patch for this bug ended up in the review queue of the
> bug mentioned earlier in the set of changesets in the review push. :(

khuey is a grumpy old man and refuses to use MozReview. So re-doing the MozReview request for just this bug's patch.
Priority: -- → P2
Comment on attachment 8709667 [details]
MozReview Request: Bug 1239899 - Pause media elements removed from document via calling pause(). r?roc

https://reviewboard.mozilla.org/r/31503/#review29029

Please explain in the commit message what was wrong with the old code.

Also, why are you deleting the web-platform-tests? That seems wrong.
Attachment #8709667 - Flags: review?(roc)
https://reviewboard.mozilla.org/r/31503/#review29029

I'm not deleting the web platfrom tests, I'm deleting the metadata files that marked the tests as expected-fail, since with my changes the tests now pass. The relevant tests still run, but now we expect them to pass.
Comment on attachment 8709667 [details]
MozReview Request: Bug 1239899 - Pause media elements removed from document via calling pause(). r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31503/diff/1-2/
Attachment #8709667 - Flags: review?(roc)
Comment on attachment 8709667 [details]
MozReview Request: Bug 1239899 - Pause media elements removed from document via calling pause(). r?roc

https://reviewboard.mozilla.org/r/31503/#review29519

Great, nthanks!
Attachment #8709667 - Flags: review?(roc) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/70629f70db00 for W(2) test failure:

Failing job (first child of your push, test didn't run for your push): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e2b96f1b426b
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=20679705&repo=mozilla-inbound

14:57:27     INFO - TEST-START | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html
14:57:37     INFO - PROCESS | 2272 | MARIONETTE LOG: INFO: Timeout fired
14:57:37     INFO - TEST-TIMEOUT | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html | paused state when removing from a document when networkState is NETWORK_EMPTY - Test timed out
14:57:37     INFO - TEST-TIMEOUT | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html | took 10126ms
14:57:37     INFO - TEST-START | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html
14:57:37     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html | paused state when removing from a document - assert_false: paused after playing expected false got true
14:57:37     INFO - v.onplaying<@http://web-platform.test:8000/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html:20:5
14:57:37     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1382:20
14:57:37     INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1406:20
14:57:37     INFO - EventHandlerNonNull*@http://web-platform.test:8000/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html:19:17
14:57:37     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1382:20
14:57:37     INFO - async_test@http://web-platform.test:8000/resources/testharness.js:513:13
14:57:37     INFO - @http://web-platform.test:8000/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html:15:1
14:57:37     INFO - TEST-OK | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html | took 230ms
Flags: needinfo?(cpearce)
Mass change P2 -> P3
Priority: P2 → P3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eb35b9811c665fec2ab1d07101d8fe83621a394

It looks like the web platform tests are green and the patches are good to go now. I will rebase the patches for review.
Flags: needinfo?(cpearce)
Attachment #8795971 - Flags: review?(cpearce)
Attachment #8795972 - Flags: review?(cpearce)
Attachment #8795973 - Flags: review?(cpearce)
Comment on attachment 8795971 [details]
Bug 1239899. Part 1 - Remove ElementInTreeState for ELEMENT_NOT_INTREE and ELEMENT_INTREE are not used at all. We can use a bool flag to do the job.

https://reviewboard.mozilla.org/r/81960/#review81178

::: dom/html/HTMLMediaElement.h:1647
(Diff revision 1)
>    nsCOMPtr<nsIPrincipal> mSrcStreamVideoPrincipal;
>  
> -  enum ElementInTreeState {
> -    // The MediaElement is not in the DOM tree now.
> -    ELEMENT_NOT_INTREE,
> -    // The MediaElement is in the DOM tree now.
> +  // True if UnbindFromTree() is called on the element.
> +  // Note this flag is false when the element is in a phase after creation and
> +  // before attaching to the DOM tree.
> +  bool mUnbindedFromTree = false;

mUnboundFromTree would be more grammatically correct. That is, we say "unbound" rather than "unbinded".
Attachment #8795971 - Flags: review?(cpearce) → review+
Comment on attachment 8795972 [details]
Bug 1239899. Part 2 - per spec. await a stable state to pause the element when it is removed from the document.

https://reviewboard.mozilla.org/r/81962/#review81180
Attachment #8795972 - Flags: review?(cpearce) → review+
Comment on attachment 8795973 [details]
Bug 1239899. Part 3 - enable tests.

https://reviewboard.mozilla.org/r/81964/#review81182

Thanks JW. I've been meaning to get back to this for ages.
Attachment #8795973 - Flags: review?(cpearce) → review+
Thanks for the review!
Assignee: nobody → jwwang
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27949d48ec66
Part 1 - Remove ElementInTreeState for ELEMENT_NOT_INTREE and ELEMENT_INTREE are not used at all. We can use a bool flag to do the job. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/a55838bd4bbb
Part 2 - per spec. await a stable state to pause the element when it is removed from the document. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/3b0e8c0d02fb
Part 3 - enable tests. r=cpearce
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: