Intermittent browser_styleeditor_bug_870339.js | leaked 2 window(s) until shutdown

RESOLVED FIXED in Firefox 39

Status

RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: RyanVM, Assigned: sjakthol)

Tracking

({intermittent-failure, memory-leak})

unspecified
Firefox 40
x86_64
macOS
intermittent-failure, memory-leak
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 wontfix, firefox39 fixed, firefox40 fixed, firefox-esr31 unaffected)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
02:51:27 WARNING - TEST-UNEXPECTED-FAIL | browser/devtools/styleeditor/test/browser_styleeditor_bug_870339.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/framework/toolbox.xul]
02:51:27 WARNING - TEST-UNEXPECTED-FAIL | browser/devtools/styleeditor/test/browser_styleeditor_bug_870339.js | leaked 2 window(s) until shutdown [url = data:text/html;charset=utf8,<!DOCTYPE%20html><html%20dir='ltr'>%20%20<head>%20%20%20%20<style>%20%20%20%20%20%20html,%20body%20{%20height:%20100%;%20}%20%20%20%20%20%20body%20{%20margin:%200;%20overflow:%20hidden;%20}%20%20%20%20%20%20.CodeMirror%20{%20width:%20100%;%20height:%20100%%20!important;%20line-height:%201.25%20!important;}%20%20%20%20</style>%20%20%20%20<link%20rel='stylesheet'%20href='chrome://browser/skin/devtools/common.css'>%20%20%20%20<link%20rel='stylesheet'%20href='chrome://browser/content/devtools/codemirror/codemirror.css'>%20%20%20%20<link%20rel='stylesheet'%20href='chrome://browser/content/devtools/codemirror/dialog.css'>%20%20%20%20<link%20rel='stylesheet'%20href='chrome://browser/content/devtools/codemirror/mozilla.css'>%20%20</head>%20%20<body%20class='theme-body%20devtools-monospace'></body></html>]
02:51:27 WARNING - TEST-UNEXPECTED-FAIL | browser/devtools/styleeditor/test/browser_styleeditor_bug_870339.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/styleeditor.xul]
02:51:27 WARNING - TEST-UNEXPECTED-FAIL | browser/devtools/styleeditor/test/browser_styleeditor_bug_870339.js | leaked 1 window(s) until shutdown [url = about:blank]
02:51:27 INFO - TEST-INFO | browser/devtools/styleeditor/test/browser_styleeditor_bug_870339.js | windows(s) leaked: [pid = 5394] [serial = 65], [pid = 5394] [serial = 63], [pid = 5394] [serial = 70], [pid = 5394] [serial = 66], [pid = 5394] [serial = 71], [pid = 5394] [serial = 67]
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
(Assignee)

Comment 4

4 years ago
Here's a patch that may fix the leaks.

It rewrites the leaking test to use openStyleEditorForURL which is quite a lot more reliable than the old open methods.

Previously the stylesheets-reset came before "editor-added" as the latter required a round-trip to the server. Bug 1147765 made the initialization a bit more sequential causing the events to be emitted in different order: first all editor-added events and then a single stylesheets-reset event. This is probably the cause for these leaks.

Here's a possible chain of events causing leaks:
- emit editor-added, test begins
- test calls _onNewDocument twice and waits for two stylesheets-reset events
- emit initial stylesheets-reset (#1)
- emit stylesheets-reset #2 => clean up and end the test
- emit stylesheets-reset for the second _onNewDocument call => add an editor => leak it

The new initialization method used in this patch properly waits for the initial stylesheets-reset event to be emitted before starting this test so the leak should be gone.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8037b20509a2
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8585131 - Flags: review?(ejpbruel)
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 on attachment 8585131 [details] [diff] [review]
bug-1148770-leaks.patch

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

Looks good to me. And Sami, let me just say that it's great that you're tackling all these issues. Keep up the good work!
Attachment #8585131 - Flags: review?(ejpbruel) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
(Reporter)

Comment 14

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/18ee8f5f75eb
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
https://hg.mozilla.org/mozilla-central/rev/18ee8f5f75eb
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
(Reporter)

Updated

4 years ago
status-firefox38: --- → affected
status-firefox39: --- → affected
status-firefox-esr31: --- → unaffected
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
seems its not fixed, still getting test failures
status-firefox40: fixed → affected
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 31

4 years ago
Well, that didn't work. Here's another idea.

StyleSheetEditor.fetchSource doesn't properly return the promise in the then handler at line 243 [1]. It causes the test to end while the promise is still pending which might keep the accompanying editor alive.

Previously the test ended a lot sooner as the no one was waiting for any promises to be fulfilled and thus that code was probably never hit as the async operations failed a lot sooner.

I taskified the method with Task.spawn and combined the two separate error handlers into one single one. They had slightly different semantics as the first one notified the user about the error but the second one didn't. Now the user is notified on both errors.

I'm shooting a bit blind here so this might not fix anything but it shouldn't make things any worse.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8934005997c

[1] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleSheetEditor.jsm?from=StyleSheetEditor.jsm#243
Attachment #8586641 - Flags: review?(ejpbruel)
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 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)
(Assignee)

Updated

4 years ago
Blocks: 1151381
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment on attachment 8586641 [details] [diff] [review]
style-editor-bug-1148770-leaks-try-2.patch

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

r+ provided this works on try
Attachment #8586641 - Flags: review?(ejpbruel) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Comment 77

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/cf861d5cdf2f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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 84

4 years ago
https://hg.mozilla.org/mozilla-central/rev/cf861d5cdf2f
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
(Assignee)

Comment 89

4 years ago
Well, that didn't seem to work either. I'll try to come up with new ideas...
status-firefox38: affected → fixed
(Reporter)

Updated

4 years ago
Status: RESOLVED → REOPENED
status-firefox39: fixed → affected
status-firefox40: fixed → affected
Resolution: FIXED → ---
Target Milestone: Firefox 40 → ---
(Reporter)

Comment 91

4 years ago
Trying to uplift to beta was a bad idea.
https://hg.mozilla.org/releases/mozilla-beta/rev/0f0c47f90ab6
status-firefox38: fixed → wontfix
Comment hidden (Legacy TBPL/Treeherder Robot)
(Assignee)

Comment 93

4 years ago
I think I finally found the real cause for this. The problem is that there's two reset operations going on the same time and the second one calls destroy on the editor added by the first one before the CodeMirror editor is loaded.

Here's what I think happens:
Op #1: call getStyleSheets, yield execution
----
Op #2: call getStyleSheets, yield execution
----
Op #1: receive style sheets
Op #1: call _resetStyleSheetList
Op #1: call _clear to clear the old editors
Op #1: call _addStyleSheet
Op #1: call _addStyleSheetEditor
Op #1: _addStyleSheetEditor creates new StyleSheetEditor and adds it to list of editors
Op #1: call fetchSource, yield execution
----
Op #2: receive style sheets
Op #2: call _resetStyleSheetList
Op #2: call _clear, the previously created StyleSheetEditor for Op#1 is destroyed (as in .destroy is called)
Op #2: continue and yield execution at some point
----
Op #1: receives the source for the style sheet
Op #1: continues and calls StyleSheetEditor.load down the road
 
At this point a new CodeMirror editor is created but as the StyleSheetEditor was already destroyed and forgotten by StyleEditorUI it won't be destroyed again. Thus the CodeMirror editor will stay alive forever leaking its parents in the process.

This patch aborts the load if the StyleSheetEditor has been destroyed. The rejection is fine as .load is only ever called from [1] and it has a rejection handler at [2].

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f037397b773

[1] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm?from=StyleEditorUI.jsm#545
[2] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm?from=StyleEditorUI.jsm#576
Attachment #8590076 - Flags: review?(bgrinstead)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment on attachment 8590076 [details] [diff] [review]
bug-1148770-try-3.patch

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

::: browser/devtools/styleeditor/StyleSheetEditor.jsm
@@ +364,5 @@
>     *         Promise that will resolve when the style editor is loaded.
>     */
>    load: function(inputElement) {
> +    if (this._isDestroyed) {
> +      return promise.reject("Won't load source editor as the style sheet has " +

Is this rejection always going to be caught?  Otherwise I think the uncaught rejection could still burn the test
(Assignee)

Comment 97

4 years ago
(In reply to Brian Grinstead [:bgrins] from comment #96)
> Comment on attachment 8590076 [details] [diff] [review]
> bug-1148770-try-3.patch
> 
> Review of attachment 8590076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleeditor/StyleSheetEditor.jsm
> @@ +364,5 @@
> >     *         Promise that will resolve when the style editor is loaded.
> >     */
> >    load: function(inputElement) {
> > +    if (this._isDestroyed) {
> > +      return promise.reject("Won't load source editor as the style sheet has " +
> 
> Is this rejection always going to be caught?  Otherwise I think the uncaught
> rejection could still burn the test

Yes. That method is only called from [1] which resides inside a task that has rejection handler at [2] (Cu.reportError).

[1] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm?from=StyleEditorUI.jsm#545
[2] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm?from=StyleEditorUI.jsm#576
Comment on attachment 8590076 [details] [diff] [review]
bug-1148770-try-3.patch

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

Nice job tracking this down
Attachment #8590076 - Flags: review?(bgrinstead) → review+
Comment hidden (Legacy TBPL/Treeherder Robot)
(Assignee)

Comment 100

4 years ago
Third time's the charm.
Keywords: checkin-needed
(Reporter)

Comment 101

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/672ac24c9269
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
(Reporter)

Comment 104

4 years ago
https://hg.mozilla.org/mozilla-central/rev/672ac24c9269
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40

Updated

9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.