Closed Bug 1041284 Opened 10 years ago Closed 10 years ago

Re-enable browser_markupview_events.js when GC issues are fixed

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 34

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(3 files, 3 obsolete files)

Test Log.test
143.50 KB, text/plain
Details
mozilla-test-fail-screenshot_t9bzob.png
698.95 KB, image/png
Details
re-enable-browser-markupview-events-js-1041284.patch
7.93 KB, patch
miker
: review+
miker
: checkin+
Details | Diff | Splinter Review
Attached file Test Log.test — 
When browser_markupview_events.js runs something makes GC kick in and clean up windows and docshells *for around 40 seconds*. This causes tasks, and eventually the test, to time out.

Because this is an issue with the testing infrastructure rather than the feature itself we will land the test disabled and fix the GC issue as part of this bug.

The relevant section of the test log along with a screenshot of the issue are attached.
This will mean going to the end of each test that creates the windows in this log and forcing GC there. It may also mean calling destroy methods at the end of those tests.

We should probably make a habit of forcing GC in tests that open a lot of windows.
These windows are GCd in the 40+ seconds pause.

103 x about:blank
75  x chrome://browser/content/devtools/inspector/inspector.xul
56  x 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>
41 x chrome://browser/content/devtools/cssruleview.xhtml
33 x chrome://browser/content/devtools/spectrum-frame.xhtml
28 x chrome://browser/content/devtools/layoutview/view.xhtml
28 x data:text/html,<html></html>
27 x chrome://browser/content/devtools/computedview.xhtml
23 x chrome://browser/content/devtools/markup-view.xhtml
20 x chrome://browser/content/devtools/cubic-bezier-frame.xhtml
20 x chrome://browser/content/devtools/fontinspector/font-inspector.xhtml
2 x data:text/html,<div%20style='width:400px;height:200px;background:yellow;'>iframe%202</div>
2 x http://example.com/browser/browser/devtools/layoutview/test/doc_layoutview_iframe1.html
2 x http://example.com/browser/browser/devtools/layoutview/test/doc_layoutview_iframe2.html
2 x http://mochi.test:8888/browser/browser/devtools/markupview/test/doc_markup_edit.html
1 x data:text/html,markup%20view%20copy%20image%20as%20data-uri
Wouldn't requesting a longer timeout be a good interim solution?
(In reply to Panos Astithas [:past] from comment #4)
> Wouldn't requesting a longer timeout be a good interim solution?

Any test that opens a few panels can trigger this so we need something more permanent.

I am going to try calling garbageCollect() on toolbox destroy... it should leave us in a much better state.
To fix the GC issues we:
- Garbage collect on toolbox close. This should help with all tests that open the toolbox.
- Manually destroy the tooltip. Allowing the browser to do it can result in intermittent leaks on Linux boxes.

Other issues fixed:
- Return early when the debug icon is clicked to avoid errors concerning this._tooltip.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=ab1a8a6d370a
Attachment #8459557 - Attachment is obsolete: true
Attachment #8460948 - Flags: review?(jwalker)
Comment on attachment 8460948 [details] [diff] [review]
re-enable-browser-markupview-events-js-1041284.patch

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

I'm not keen on us adding explicit GC to live code just so we can pass tests. That feels like it's making things worse for the user.

Also I don't think we should be adding events for destroy actions. Can't we use a promise?
Attachment #8460948 - Flags: review?(jwalker)
(In reply to Joe Walker [:jwalker] from comment #8)
> Comment on attachment 8460948 [details] [diff] [review]
> re-enable-browser-markupview-events-js-1041284.patch
> 
> Review of attachment 8460948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not keen on us adding explicit GC to live code just so we can pass
> tests. That feels like it's making things worse for the user.
> 

I have added the conditional if (gDevTools.testing) {...} so that there is no action when a user closes the toolbox but we still avoid building a large GC backlog in tests.

> Also I don't think we should be adding events for destroy actions. Can't we
> use a promise?

We don't even need to do that. As long as tooltip.hide() is called the test runs just fine.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=99622a723f67
Attachment #8460948 - Attachment is obsolete: true
Attachment #8467959 - Flags: review?(jwalker)
Comment on attachment 8467959 [details] [diff] [review]
re-enable-browser-markupview-events-js-1041284.patch

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

::: browser/devtools/markupview/test/head.js
@@ +13,5 @@
>  waitForExplicitFinish();
>  
> +// If a test times out we want to see the complete log and not just the last few
> +// lines.
> +SimpleTest.requestCompleteLog();

Does this still need to be here?

::: browser/devtools/shared/widgets/Tooltip.js
@@ +1203,5 @@
>  
>    _headerClicked: function(event) {
>      if (event.target.classList.contains("event-tooltip-debugger-icon")) {
>        this._debugClicked(event);
> +      return true;

Wouldn't event.stopPropagation() be clearer?
Attachment #8467959 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #10)
> Comment on attachment 8467959 [details] [diff] [review]
> re-enable-browser-markupview-events-js-1041284.patch
> 
> Review of attachment 8467959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/markupview/test/head.js
> @@ +13,5 @@
> >  waitForExplicitFinish();
> >  
> > +// If a test times out we want to see the complete log and not just the last few
> > +// lines.
> > +SimpleTest.requestCompleteLog();
> 
> Does this still need to be here?
> 

I left it there because in the event of a timeout we want to see as much of the log as possible and not just the last few lines. We discussed this in a daily meeting and the consensus was that this should probably be default for our tests.

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +1203,5 @@
> >  
> >    _headerClicked: function(event) {
> >      if (event.target.classList.contains("event-tooltip-debugger-icon")) {
> >        this._debugClicked(event);
> > +      return true;
> 
> Wouldn't event.stopPropagation() be clearer?

Changed as requested.
Attachment #8467959 - Attachment is obsolete: true
Attachment #8469619 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/77bf5dca7720
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
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: