1386550 - Stop using sdk/system/events in DevTools

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P1)

Description

[Soledad Penades [:sole] [:spenades]]

Looks like it's only on a test file: http://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/addon-source/browser_dbg_addon3/lib/main.js#2

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1386550 - remove usage of sdk/system/events in DevTools test;

Review commit: https://reviewboard.mozilla.org/r/163834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163834/

Comment 2

[Julian Descottes [:jdescottes]]

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.

Comment 3

[Soledad Penades [:sole] [:spenades]]

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!

Comment 4

[Julian Descottes [:jdescottes]]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6854ec0231cc
remove usage of sdk/system/events in DevTools test;r=sole

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6854ec0231cc

Comment 7

[Alexandre Poirot [:ochameau]]

(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