Closed Bug 1173196 Opened 4 years ago Closed 4 years ago

Intermittent browser_toolbox_minimize.js | Test timed out


(DevTools :: Framework, defect)

Not set


(firefox39 unaffected, firefox40 unaffected, firefox41 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected)

Firefox 41
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected


(Reporter: KWierso, Assigned: sjakthol)


(Keywords: intermittent-failure)


(2 files)

12:24:10 INFO - MEMORY STAT heapAllocated after test: 80936820
12:24:10 INFO - 847 INFO TEST-OK | browser/devtools/framework/test/browser_toolbox_hosts_size.js | took 3832ms
12:24:10 INFO - 848 INFO TEST-START | browser/devtools/framework/test/browser_toolbox_minimize.js
12:25:40 INFO - Xlib: extension "RANDR" missing on display ":0".
12:25:42 INFO - TEST-INFO | screentopng: exit 0
12:25:42 INFO - 849 INFO checking window state
12:25:42 INFO - 850 INFO Entering test
12:25:42 INFO - 851 INFO Create a test tab and open the toolbox
12:25:42 INFO - 852 INFO Adding a new tab with URL: 'data:text/html;charset=utf8,test page'
12:25:42 INFO - 853 INFO URL 'data:text/html;charset=utf8,test page' loading complete
12:25:42 INFO - 854 INFO TEST-PASS | browser/devtools/framework/test/browser_toolbox_minimize.js | The minimize button exists in the default bottom host
12:25:42 INFO - 855 INFO Try to minimize the toolbox
12:25:42 INFO - 856 INFO TEST-PASS | browser/devtools/framework/test/browser_toolbox_minimize.js | The toolbox host has been hidden away with a negative-margin
12:25:42 INFO - 857 INFO Try to maximize again the toolbox
12:25:42 INFO - 858 INFO Console message: 1433877880841 Toolkit.GMP WARN GMPInstallManager.parseResponseXML got node name: html, expected: updates
12:25:42 INFO - 859 INFO Console message: [JavaScript Error: "1433877880842 Toolkit.GMP ERROR GMPInstallManager.simpleCheckAndInstall Could not check for addons: {"target":{},"message":"got node name: html, expected: updates"}" {file: "resource://gre/modules/Log.jsm" line: 749}]
12:25:42 INFO - 860 INFO Console message: 1433877880921 Services.HealthReport.HealthReporter WARN Saved state file does not exist.
12:25:42 INFO - 861 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_minimize.js | Test timed out - expected PASS
12:25:42 INFO - MEMORY STAT vsize after test: 638693376
12:25:42 INFO - MEMORY STAT residentFast after test: 196718592
12:25:42 INFO - MEMORY STAT heapAllocated after test: 83923480
12:25:42 INFO - 862 INFO TEST-OK | browser/devtools/framework/test/browser_toolbox_minimize.js | took 90162ms
12:25:42 INFO - 863 INFO TEST-START | browser/devtools/framework/test/browser_toolbox_options.js
I don't understand what's happening exactly but it seems like the ontransitionend event listener used in toolbox-host.js to detect when the host becomes maximized again doesn't work.
I've tried just adding the equivalent of an executeSoon after the minimized and maximized events were received, in the test, and that seems to work:
Adding the listener before setting the margin seems to fix this.

I think that when the margin is set the browser triggers computation (i.e. GC or something) that exceeds the time it takes to do the transition. This causes 'transitionend' event to come before the script execution continues and the handler is added.

Waiting for a tick gives the browser a chance to do the computation earlier so it won't block the script execution when the margin is set.

As a more general thought, it's always a good idea to attach a listener for an event before you execute the code that triggers the event.

Assignee: nobody → sjakthol
Attachment #8621948 - Flags: review?(pbrosset)
Comment on attachment 8621948 [details] [diff] [review]

Review of attachment 8621948 [details] [diff] [review]:

Oh, great catch, of course that's the problem. Thanks for finding that out.
Attachment #8621948 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Is this something that would be useful to backport?
Flags: needinfo?(sjakthol)
I think this just landed so it's only in Nightly.
Flags: needinfo?(sjakthol)
Well, that didn't help much. This was the easy fix, now it starts to get more interesting. However, I don't really have time to look at this during the next few days so lets hope that the patch here helped even a little.
Resolution: FIXED → ---
Another idea I had that was that the code accepts a transitionend event for all properties - not just 'margin-bottom' it triggers. So I did a try run with a debugging patch and it revealed that indeed, the first 'minimize' event fired too soon due to a transitionend event for 'backgroud-color' [1]. This apparently caused something to go wrong when the test continued to maximize the toolbox again.

This patch makes the code to ignore transitionend events for properties other than margin-bottom.


Attachment #8623652 - Flags: review?(pbrosset)
Attachment #8621948 - Flags: checkin+
Comment on attachment 8623652 [details] [diff] [review]

Review of attachment 8623652 [details] [diff] [review]:

Cool. Thanks Sami.
Attachment #8623652 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Closed: 4 years ago4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.