Stop using sdk/system/events in DevTools

RESOLVED FIXED in Firefox 57

Status

P1
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: sole, Assigned: jdescottes)

Tracking

unspecified
Firefox 57
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-nosdk])

Attachments

(1 attachment)

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
mozreview-review
Comment on attachment 8892833 [details]
Bug 1386550 - remove usage of sdk/system/events in DevTools test;

https://reviewboard.mozilla.org/r/163834/#review169166

::: devtools/client/debugger/test/mochitest/addon-source/browser_dbg_addon3/lib/main.js
(Diff revision 1)
>    observe: function () {
>      debugger;
>    }
>  };
>  
> -once("sdk:loader:destroy", () => observerService.removeObserver(observer, "debuggerAttached"));

To be totally honest I tried to remove this line, and no test seemed to fail ... In debug/non-debug/e10s/non-e10s. No leak detected either. So I'm not 100% sure why this was needed before.

I have a try push ongoing at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94204c5f499565ef88681ea1a0230d9f75fe96ba but debug builds failed to start a few times. Looks greenish in non-debug though.
Whiteboard: [nosdk]
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [nosdk] → [reserve-nosdk]
Target Milestone: --- → Firefox 57
Flags: qe-verify? → qe-verify-
(Reporter)

Comment 3

a year ago
mozreview-review
Comment on attachment 8892833 [details]
Bug 1386550 - remove usage of sdk/system/events in DevTools test;

https://reviewboard.mozilla.org/r/163834/#review169280

This looks good to me initially...

I searched for the sdk:loader:destroy string and found this in our code path: http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#206 - it seems to be related to the addon workflow? I don't know if that workflow is tested, or if we're just piggybacking on this event for the addon workflow. It might be worth getting a second review / confirmation from Alex.

Matteo also looked at this and thought that this loader refers to the Addon SDK only, not the DevTools one, but now I'm not so sure!
Attachment #8892833 - Flags: review?(spenades) → review+
I can be wrong, but our loader uses the sdk loader, so "sdk:loader:destroy" might be fired by our loader or any other loader using resource://gre/modules/commonjs/toolkit/loader.js.

The code at http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#206 seems wrong, this event could be fired by any other Loader, not only our Loader. But I don't know much about this area.

I'll wait to see what Alex has to say about this topic, but there should not be any connection to the patch being reviewed here.
Flags: needinfo?(poirot.alex)

Comment 5

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6854ec0231cc
remove usage of sdk/system/events in DevTools test;r=sole

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6854ec0231cc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
(In reply to Julian Descottes [:jdescottes] from comment #4)
> I can be wrong, but our loader uses the sdk loader, so "sdk:loader:destroy"
> might be fired by our loader or any other loader using
> resource://gre/modules/commonjs/toolkit/loader.js.

True.

> The code at
> http://searchfox.org/mozilla-central/source/devtools/client/framework/
> devtools-browser.js#206 seems wrong, this event could be fired by any other
> Loader, not only our Loader. But I don't know much about this area.

Yes, the event can be fired any other loader instances, for SDK addons for example.

http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#207-211
        // This event is fired when the devtools loader unloads, which happens
        // only when the add-on workflow ask devtools to be reloaded.
        if (subject.wrappedJSObject == require("@loader/unload")) {
          gDevToolsBrowser.destroy({ shuttingDown: false });
        }
But this code is correct thanks to this check: subject.wrappedJSObject == require("@loader/unload")
which is a typical SDK complex things, where we ensure the given event is the one for our loader, the DevTools one.

http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#1136-1147
http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#1071-1072
Flags: needinfo?(poirot.alex)

Updated

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