1363419 - DevTools add-on should cleanup on disabled/uninstall

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

For now, the DevTools add-on only cleans up when pressing Ctrl+Alt+R key shortcut.
But it should also cleanup DevTools when the add-on is disabled.
Cleanup means, closing all toolboxes and remove menu as well as key shortuts.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1363419 - Unregister DevTools on Add-on disabling.

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

Comment 2

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a54b02d5c9f986237d16d769fdf7fc38a301e799

Comment 3

[Patrick Brosset <:pbro>]

DevTools bug triage (filter on CLIMBING SHOES).

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8865921 [details]
Bug 1363419 - Unregister DevTools on Add-on disabling.

https://reviewboard.mozilla.org/r/137508/#review141336

The code change itself looks good, I just have a doubt regarding calling unload() from the shudown() method. 
Not sure if my concerns are grounded, so I'll let you have a look and decide if it's relevant or not.

::: devtools/bootstrap.js:155
(Diff revision 1)
>      obs.notifyObservers(null, "message-manager-flush-caches");
>  
>      /* Also purge cached modules in child processes, we do it a few lines after
>         in the parent process */
>      if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) {
>        Services.obs.notifyObservers(null, "devtools-unload", "reload");

I tried to track how this "reload" data parameter had any impact on the unload. I think the only "special" value is "shutdown" [1] ? So any value other that "shutdown" should have the same effect.

For total correctness, maybe we should have different data parameters when unload() is called from reload() or from shutdown().  

[1] http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/system/events.js#165-167

::: devtools/bootstrap.js:313
(Diff revision 1)
>      if (userValue == prefs[name]) {
>        Services.prefs.setBoolPref(name, originalPrefValues[name]);
>      }
>    }
> +
> +  unload();

shutdown() is called in 3 situations, according to MDN:
- When the extension is uninstalled, if it's currently enabled.
- When the extension becomes disabled.
- When the user quits the application, if the extension is enabled.

The two first cases are fine, but what is the impact of calling the addon unload() when quitting Firefox? 

Today, I don't think we run explicitly a similar shutdown code when Firefox closes? Will this represent additional processing? Could it slow down the closing of FF? 

Normally the shutdown method receives a reason as 2nd argument. Should we avoid calling our unload() if the reason is APP_SHUTDOWN?

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #4)
> Comment on attachment 8865921 [details]
> 
> ::: devtools/bootstrap.js:155
> (Diff revision 1)
> >      obs.notifyObservers(null, "message-manager-flush-caches");
> >  
> >      /* Also purge cached modules in child processes, we do it a few lines after
> >         in the parent process */
> >      if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) {
> >        Services.obs.notifyObservers(null, "devtools-unload", "reload");
> 
> I tried to track how this "reload" data parameter had any impact on the
> unload. I think the only "special" value is "shutdown" [1] ? So any value
> other that "shutdown" should have the same effect.
> 
> For total correctness, maybe we should have different data parameters when
> unload() is called from reload() or from shutdown().  
> 
> [1]
> http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/system/
> events.js#165-167

Yes. But we could use it here to make a useful distinction:
http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#205-209
I'll at least pass a meaningful reason here and not always "reload", even if listening code doesn't use it.


> Today, I don't think we run explicitly a similar shutdown code when Firefox
> closes? Will this represent additional processing? Could it slow down the
> closing of FF? 

We do run some cleanup, similar one:
http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#201-202
forgetBrowserWindow is going to unregister menus for ex.

> 
> Normally the shutdown method receives a reason as 2nd argument. Should we
> avoid calling our unload() if the reason is APP_SHUTDOWN?

But I agree, this existing cleanup in devtools-browser is historical,
I'll try to avoid doing redundant/unecessary cleanup here on shutdown.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8865921 [details]
Bug 1363419 - Unregister DevTools on Add-on disabling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137508/diff/1-2/

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8865921 [details]
Bug 1363419 - Unregister DevTools on Add-on disabling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137508/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20977f40cc82
Unregister DevTools on Add-on disabling. r=jdescottes

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20977f40cc82