Closed Bug 1386357 Opened 7 years ago Closed 7 years ago

Stop using sdk/timers in DevTools codebase

Categories

(DevTools :: General, enhancement, P1)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ochameau, Assigned: jdescottes)

References

Details

(Whiteboard: [reserve-nosdk])

Attachments

(2 files)

Bug 1386357 - remove usage of sdk/timers in DevTools;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1386357 - add new debugger head to support files for browser_browser_toolbox_debugger.js;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Whiteboard: [nosdk]
search: http://searchfox.org/mozilla-central/search?q=sdk%2Ftimers&path=devtools%2F

files: 
* devtools/client/canvasdebugger/test/head.js
* devtools/client/framework/test/browser_browser_toolbox_debugger.js
* devtools/server/tests/unit/test_promises_object_timetosettle-02.js
* devtools/shared/discovery/tests/unit/test_discovery.js
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9adf6312b79ca88de93ca65f7f710ec60fbc6b94

For the record devtools/client/framework/test/browser_browser_toolbox_debugger.js is permafailing for me locally, both with or without my change.
Whiteboard: [nosdk] → [nosdk] [triage]
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [nosdk] [triage] → [reserve-nosdk]
Target Milestone: --- → Firefox 57
Flags: qe-verify? → qe-verify-
ochameau: 

Following our chat on #devtools about tests failing in isolation because devtools is not initialized, I checked for devtools/client/framework/test/browser_browser_toolbox_debugger.js ... and it still fails even if devtools init is forced in devtools-startup.
Comment on attachment 8892840 [details]
Bug 1386357 - remove usage of sdk/timers in DevTools;

https://reviewboard.mozilla.org/r/163852/#review169644

::: devtools/client/canvasdebugger/test/head.js:9
(Diff revision 1)
>  
>  var { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
>  
>  var { generateUUID } = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>  var { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +var { setTimeout } = Cu.import("resource://gre/modules/Timer.jsm", {});

From mochitest, you should be able to pull setTimeout from window:
var { setTimeout } = window;
In head.js, you don't have setTimeout set as global, it is unclear why, but you do have it in test scripts as it evaluates in the scope of the top level browser window and have access to all its globals...

Feel free to keep the Timer.jsm if you find it better.

::: devtools/client/framework/test/browser_browser_toolbox_debugger.js:13
(Diff revision 1)
>  // chrome://mochitests/ package isn't available from browser toolbox process.
>  
>  // On debug test runner, it takes about 50s to run the test.
>  requestLongerTimeout(4);
>  
> -const { setInterval, clearInterval } = require("sdk/timers");
> +const { setInterval, clearInterval } = Cu.import("resource://gre/modules/Timer.jsm", {});

As this is a mochitest test script, this import is useless, you already have setInterval/clearInteval in this scope.
Attachment #8892840 - Flags: review?(poirot.alex) → review+
(In reply to Julian Descottes [:jdescottes] from comment #3)
> For the record
> devtools/client/framework/test/browser_browser_toolbox_debugger.js is
> permafailing for me locally, both with or without my change.

I got it to fail only once, the first time I tried it :/

Is it a permafail for you?

I got this failure:
4 INFO TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_browser_toolbox_debugger.js | Uncaught exception - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:453 - Error: Failed to fetch chrome://mochitests/content/browser/devtools/client/framework/test/../../debugger/new/test/mochitest/head.js. Code 2152857618.
Comment on attachment 8892840 [details]
Bug 1386357 - remove usage of sdk/timers in DevTools;

https://reviewboard.mozilla.org/r/163852/#review169644

> From mochitest, you should be able to pull setTimeout from window:
> var { setTimeout } = window;
> In head.js, you don't have setTimeout set as global, it is unclear why, but you do have it in test scripts as it evaluates in the scope of the top level browser window and have access to all its globals...
> 
> Feel free to keep the Timer.jsm if you find it better.

Seems like setTimeout is actually defined in head.js ? I tried dump()ing it early and it seems to be defined from the beginning.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d08a95cd7559a983a08fc096d5d244eb6e1e6a
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> (In reply to Julian Descottes [:jdescottes] from comment #3)
> > For the record
> > devtools/client/framework/test/browser_browser_toolbox_debugger.js is
> > permafailing for me locally, both with or without my change.
> 
> I got it to fail only once, the first time I tried it :/
> 
> Is it a permafail for you?
> 
> I got this failure:
> 4 INFO TEST-UNEXPECTED-FAIL |
> devtools/client/framework/test/browser_browser_toolbox_debugger.js |
> Uncaught exception - at resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/shared/DevToolsUtils.js:453 - Error: Failed to fetch
> chrome://mochitests/content/browser/devtools/client/framework/test/../../
> debugger/new/test/mochitest/head.js. Code 2152857618.

Yes same failure, and it seems to consistently fail for me. I will investigate.
Adding the new debugger head file to the support files for this test suite fixes the issue for me.
Comment on attachment 8893376 [details]
Bug 1386357 - add new debugger head to support files for browser_browser_toolbox_debugger.js;

https://reviewboard.mozilla.org/r/164482/#review169760

::: devtools/client/debugger/new/test/mochitest/head.js:40
(Diff revision 1)
>  // shared-head.js handles imports, constants, and utility functions
>  Services.scriptloader.loadSubScript(
>    "chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js",
>    this
>  );
> +// yolo

It looks like you should look into pre-commit hooks to prevent these things!

https://github.com/git/git/blob/master/templates/hooks--pre-commit.sample

::: devtools/client/framework/test/browser.ini:45
(Diff revision 1)
>    browser_toolbox_options_enable_serviceworkers_testing.html
>    serviceworker.js
>    sjs_code_reload.sjs
>    sjs_code_bundle_reload_map.sjs
>    test_browser_toolbox_debugger.js
> +  !/devtools/client/debugger/new/test/mochitest/head.js

I'm wondering if we can have the same error because of shared-head also missing from supports-file?
Why does it work fine with shared-head and not debugger's head?!
Attachment #8893376 - Flags: review?(poirot.alex) → review+
Comment on attachment 8893376 [details]
Bug 1386357 - add new debugger head to support files for browser_browser_toolbox_debugger.js;

https://reviewboard.mozilla.org/r/164482/#review169760

> It looks like you should look into pre-commit hooks to prevent these things!
> 
> https://github.com/git/git/blob/master/templates/hooks--pre-commit.sample

Damn... I blame the weather.

> I'm wondering if we can have the same error because of shared-head also missing from supports-file?
> Why does it work fine with shared-head and not debugger's head?!

shared-head is in the same folder and is already in supports-file: http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/devtools/client/framework/test/browser.ini#34
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de883798a9c4
remove usage of sdk/timers in DevTools;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/dbc1163e84d5
add new debugger head to support files for browser_browser_toolbox_debugger.js;r=ochameau
(In reply to Julian Descottes [:jdescottes] from comment #14)
> > I'm wondering if we can have the same error because of shared-head also missing from supports-file?
> > Why does it work fine with shared-head and not debugger's head?!
> 
> shared-head is in the same folder and is already in supports-file:
> http://searchfox.org/mozilla-central/rev/
> f0e4ae5f8c40ba742214e89aba3f554da0b89a33/devtools/client/framework/test/
> browser.ini#34

As debugger's head.js?
http://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser.ini#130
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: