DevTools add-on should cleanup on disabled/uninstall

RESOLVED FIXED in Firefox 55

Status

P3
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request)
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P3

Comment 4

2 years ago
mozreview-review
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?
Attachment #8865921 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 5

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1364063
Comment hidden (mozreview-request)

Comment 8

2 years ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20977f40cc82
Unregister DevTools on Add-on disabling. r=jdescottes

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20977f40cc82
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

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