Closed Bug 1303611 Opened 8 years ago Closed 8 years ago

Inspector high cpu and memory usage (leak?) on twitch player page leading to firefox hang

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Version:
51 Branch
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: kasper93, Assigned: jdescottes)

Details

Attachments

(3 files)

Screenshot with the issue.
242.16 KB, image/png
Details
The same workload without devtools open
1000.33 KB, image/png
Details
Bug 1303611 - cache value on attribute container to avoid redrawing unchanged attributes;
58 bytes, text/x-review-board-request
pbro
: review+
Details 
Go to page with twitch player http://player.twitch.tv/?channel={any channel you want}

Open devtool inspector. Instantly cpu and memory usage goes up and parameters in inspector are highlighted as they were constantly changed.

See attached screenshot. This is main firefox process. CPU usage is not caused by decoding because this was done by flash. Note the drop in memory usage in the end, this is the point that firefox started freezing and couldn't play the video anymore.
Hi Kacper,
I can confirm this issue with the following Browser:

FF 49.0 (Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:49.0) Gecko/20100101 Firefox/49.0)
Build ID: 20160919213653
running on Ubuntu LTS Xenial; 32 bit (x86)

But I would extend your bug description:
Steps:
Open FF and navigate to any Twitch channel,
open Developer Tools (on any area, but not (!) on the video) --> CPU usage goes up to ~40% (before ~15%)
open Developer Tools (by choosing "inspect element" on the video) --> CPU goes up to ~80+%

I recieved the following output on the debug console (firefox --debug):
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: parent is null
Full stack: MarkupView.prototype._isImagePreviewTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:495:1
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype.isValidHoverTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:147:21
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype._onMouseMove/this.toggleTimer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:127:9

*************************
console.error: 
  isValidHoverTarget rejected with unexpected reason:
console.error: 
  Message: TypeError: parent is null
  Stack:
    MarkupView.prototype._isImagePreviewTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:495:1
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype.isValidHoverTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:147:21
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype._onMouseMove/this.toggleTimer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:127:9

*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: parent is null
Full stack: MarkupView.prototype._isImagePreviewTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:495:1
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype.isValidHoverTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:147:21
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype._onMouseMove/this.toggleTimer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:127:9

*************************
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: parent is null
Full stack: MarkupView.prototype._isImagePreviewTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:495:1
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype.isValidHoverTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:147:21
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype._onMouseMove/this.toggleTimer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:127:9

*************************
console.error: 
  isValidHoverTarget rejected with unexpected reason:
console.error: 
  Message: TypeError: parent is null
  Stack:
    MarkupView.prototype._isImagePreviewTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:495:1
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype.isValidHoverTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:147:21
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype._onMouseMove/this.toggleTimer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:127:9

[Thread 0x903b8b40 (LWP 5216) exited]
[New Thread 0x903b8b40 (LWP 5243)]
[Thread 0x903b8b40 (LWP 5243) exited]
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: parent is null
Full stack: MarkupView.prototype._isImagePreviewTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:495:1
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype.isValidHoverTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:147:21
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype._onMouseMove/this.toggleTimer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:127:9

*************************
*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: parent is null
Full stack: MarkupView.prototype._isImagePreviewTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:495:1
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype.isValidHoverTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:147:21
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype._onMouseMove/this.toggleTimer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:127:9

*************************
console.error: 
  isValidHoverTarget rejected with unexpected reason:
console.error: 
  Message: TypeError: parent is null
  Stack:
    MarkupView.prototype._isImagePreviewTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/markup/markup.js:495:1
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype.isValidHoverTarget<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:147:21
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
TooltipToggle.prototype._onMouseMove/this.toggleTimer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/TooltipToggle.js:127:9

[New Thread 0x903b8b40 (LWP 5267)]

Thread 8 "Socket Thread" received signal SIGPIPE, Broken pipe.
[Switching to Thread 0xac6feb40 (LWP 4860)]
0xb7fdac31 in __kernel_vsyscall ()
Inspector bug triage (filter on CLIMBING SHOES).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Performance regression from Bug 1225063 (landed in Firefox 47). 

When updating a container, the markup-view normally avoids to recreate "attribute editor" elements for unchanged attributes.
The current value is expected to be stored in "dataset.value" of the attribute editor element, but it was never set. So the comparison with the new value always failed.

In the case of the twitch player, a node with a lot of attributes was regularly updated, thus triggering a huge amount of DOM modifications in the markup view.
Green try (except for unrelated intermittents): https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcaac33a1032
Attachment #8796768 - Flags: review?(bgrinstead) → review?(pbrosset)
Looks like a really good, simple one-liner fix.  Bouncing the review to Patrick though, just to double check
Comment on attachment 8796768 [details]
Bug 1303611 - cache value on attribute container to avoid redrawing unchanged attributes;

https://reviewboard.mozilla.org/r/82506/#review81652

::: devtools/client/inspector/markup/test/browser_markup_mutation_01.js:76
(Diff revision 1)
> +  },
> +  {
> +    desc: "Adding another attribute does not rerender unchanged attributes",
> +    test: function* (testActor, inspector) {
> +      let {editor} = yield getContainerForSelector("#node1", inspector);
> +      // Set an attribute on the node before starting the test.

This test sets attributes on both the markup-view elements, and on the content elements, so for someone reading this test for the first time, it may be confusing what is doing what, especially if you don't know much about e10s and the testActor.
I think you could maybe clarify this a bit in this comment.

::: devtools/client/inspector/markup/test/browser_markup_mutation_01.js:77
(Diff revision 1)
> +  {
> +    desc: "Adding another attribute does not rerender unchanged attributes",
> +    test: function* (testActor, inspector) {
> +      let {editor} = yield getContainerForSelector("#node1", inspector);
> +      // Set an attribute on the node before starting the test.
> +      [...editor.attrList.querySelectorAll(".attreditor")].forEach(attr => {

I know this is how other items in the test array do it too, but I'm wondering why we use a forEach here. It seems like we're really interested only in testing just 1 attribute, and that's the newattr one.

Would this work?

```
editor.attrList.querySelector([data-attr=newattr]);
```

This would avoid to have to loop over each attribute node in the container. Not that it's wrong, but it's just harder to read I guess.

::: devtools/client/inspector/markup/test/browser_markup_mutation_01.js:87
(Diff revision 1)
> +      // Add another attribute
> +      yield testActor.setAttribute("#node1", "otherattr", "othervalue");
> +    },
> +    check: function* (inspector) {
> +      let {editor} = yield getContainerForSelector("#node1", inspector);
> +      ok([...editor.attrList.querySelectorAll(".attreditor")].some(attr => {

Same here. But since other parts of this test do this too, I'm fine if you prefer to stay consistent.
Attachment #8796768 - Flags: review?(pbrosset) → review+
Comment on attachment 8796768 [details]
Bug 1303611 - cache value on attribute container to avoid redrawing unchanged attributes;

https://reviewboard.mozilla.org/r/82506/#review81652

Thanks for the review! 

Rewrote the new test I added in a more descriptive and flat style, hopefully easier to understand.

> I know this is how other items in the test array do it too, but I'm wondering why we use a forEach here. It seems like we're really interested only in testing just 1 attribute, and that's the newattr one.
> 
> Would this work?
> 
> ```
> editor.attrList.querySelector([data-attr=newattr]);
> ```
> 
> This would avoid to have to loop over each attribute node in the container. Not that it's wrong, but it's just harder to read I guess.

Yep, works and suits this test fine.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88a8ec488902
cache value on attribute container to avoid redrawing unchanged attributes;r=pbro
https://hg.mozilla.org/mozilla-central/rev/88a8ec488902
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: