Closed Bug 1386550 Opened 4 years ago Closed 4 years ago

Stop using sdk/system/events in DevTools

Categories

(DevTools :: General, enhancement, P1)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: sole, Assigned: jdescottes)

References

Details

(Whiteboard: [reserve-nosdk])

Attachments

(1 file)

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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-
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)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6854ec0231cc
remove usage of sdk/system/events in DevTools test;r=sole
https://hg.mozilla.org/mozilla-central/rev/6854ec0231cc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
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)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.