Closed Bug 1148770 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: Style Editor, defect)

Product:
DevTools
Component:
Style Editor
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
RESOLVED FIXED
Milestone:
Firefox 40
Tracking Flags:
Tracking Status
firefox38 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: sjakthol)

References

Details

(Keywords: intermittent-failure, memory-leak)

Attachments

(3 files)

bug-1148770-leaks.patch
3.17 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
style-editor-bug-1148770-leaks-try-2.patch
2.53 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
bug-1148770-try-3.patch
1.28 KB, patch
bgrins
: review+
Details | Diff | Splinter Review 
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]
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 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+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/18ee8f5f75eb
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/18ee8f5f75eb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
seems its not fixed, still getting test failures
status-firefox40: fixed → affected
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Blocks: 1151381
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf861d5cdf2f
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Well, that didn't seem to work either. I'll try to come up with new ideas...
Status: RESOLVED → REOPENED
status-firefox39: fixed → affected
status-firefox40: fixed → affected
Resolution: FIXED → ---
Target Milestone: Firefox 40 → ---
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 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
(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+
Third time's the charm.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/672ac24c9269
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
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: