Cannot reload Developer Tool with DevTools Reload

RESOLVED FIXED in Firefox 48

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: r_kato, Assigned: ochameau)

Tracking

unspecified
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [btpp-backlog])

Attachments

(3 attachments, 5 obsolete attachments)

Reporter

Description

3 years ago
Posted image 2016-04-03.png
Nightly: 48.0a1 (2016-04-02)
gecko-dev repository: 1bb567e
https://github.com/mozilla/gecko-dev/commit/1bb567e688ffbcb0939974b6717bd97dbecee319

Step to reproduce:
1. Load DevTools Reload's install.rdf
2. Hit F12 to open devtool, and hit Ctrl+Alt+R

Actual result:
Nothing happened.
My Browser Console says an error thrown (please see the attached file).
Reporter

Updated

3 years ago
Summary: Cannot reload DevTools Reload → Cannot reload Developer Tool with DevTools Reload

Updated

3 years ago
Flags: needinfo?(sole)
Thanks for the ping - I will reroute to the developers working on it which will take good care of it :-)
Flags: needinfo?(sole) → needinfo?(poirot.alex)
Assignee

Comment 2

3 years ago
Thanks for the report, I can repro. Something regressed recently.
Assignee: nobody → poirot.alex
Flags: needinfo?(poirot.alex)
Assignee

Updated

3 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 3

3 years ago
Posted patch wip (obsolete) — Splinter Review
Work in progress. It looks like JSM have a very different behavior when used
in packaged builds. It seems to have changed. May be the addon manager flush
JSM/chrome caches more agressively.
From bootstrap.js, we no longer get access to the previous Loader.jsm instance
and always get access to the new one.
Priority: -- → P3
Whiteboard: [btpp-backlog]
Assignee

Comment 5

3 years ago
My patchs depend on bug 1258546, which itself depends on bug 1258496.
If the platform patch from bug 1258496 doesn't move forward quickly, I'll try to reverse this dependency.
Depends on: 1258546
Assignee

Comment 6

3 years ago
A followup from a previous cleanup. bug 1247203 comment 30.
It looks like main.js is more disturbing then really helpful.
It is especially disturbing in Loader.jsm. It is mostly used to defined
what is exported *for compatiblity reasons* out of Loader.jsm.
It is less disturbing for the SDK loader but still I think
it is more related to addon concept than our usage.
We couldn't really reload these main modules as-is.
There is a lot of stuff around them that needs to be set/unset.

What do you think? If you don't get a good feeling about this patch I'll just drop it and focus on what is actually really necessary.
Here the way to unload/reload the previously loaded module loader is what fixes the original issue behind this bug report.
(devtools-unload event)
Attachment #8739212 - Flags: review?(jryans)
Assignee

Updated

3 years ago
Attachment #8738092 - Attachment is obsolete: true
Assignee

Comment 7

3 years ago
This is another important fix after this regression.
If we try to use Loader.jsm in bootstrap.js, it will actually load
a brand new instance with the fresh sources reference by chrome.manifest of the addon.
It wasn't doing that when I introduced the addon...

So we shouldn't depend on any devtools module before doing the reload!
Attachment #8739214 - Flags: review?(jryans)
Assignee

Comment 8

3 years ago
The browser console also needs to be closed and reloaded differently.
I think it would be better if HUDService closes it automatically when
the modules are unloaded, but it doesn't do that for now.

Anyway, I think it is still bootstrap.js responsibility to reopen it.
Attachment #8739215 - Flags: review?(jryans)
Attachment #8739214 - Flags: review?(jryans) → review+
Comment on attachment 8739215 [details] [diff] [review]
fix browser console reload via the addon - v1

Review of attachment 8739215 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/bootstrap.js
@@ +168,5 @@
>  
> +  // Browser console document can't just be reloaded.
> +  // HUDService is going to close it on unload.
> +  // Instead we have to manually toggle it.
> +  if (reopenBrowserConsole) {

Shouldn't this patch be merged with the previous one?  `reopenBrowserConsole` is introduced in the previous patch, and is not used anywhere there...
Attachment #8739215 - Flags: review?(jryans) → review+
Comment on attachment 8739212 [details] [diff] [review]
Remove unecessary Loader.jsm main in favor of require() - v1

Review of attachment 8739212 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks good overall, but who is using the "reload" data?

::: devtools/bootstrap.js
@@ +99,5 @@
>    }, false);
>  
> +  // As we can't get a reference to existing Loader.jsm instances, we send them
> +  // an observer service notification to unload them.
> +  Services.obs.notifyObservers(null, "devtools-unload", "reload");

Who is using this "reload" data?

::: devtools/server/content-server.jsm
@@ +19,5 @@
>    // in the same process.
>    let devtools = new DevToolsLoader();
>    devtools.invisibleToDebugger = true;
> +  let main = devtools.require("devtools/server/main");
> +  let { DebuggerServer, ActorPool } = main;

Might as well combine these lines?

::: devtools/shared/Loader.jsm
@@ +130,5 @@
>  var gNextLoaderID = 0;
>  
>  /**
>   * The main devtools API.
> + * The standard instance of this loader is

Nit: rewrap the remaining lines below
Attachment #8739212 - Flags: review?(jryans) → feedback+
Assignee

Updated

3 years ago
Attachment #8739214 - Attachment is obsolete: true
Attachment #8739215 - Attachment is obsolete: true
Assignee

Comment 12

3 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> Comment on attachment 8739212 [details] [diff] [review]
> Remove unecessary Loader.jsm main in favor of require() - v1
> 
> Review of attachment 8739212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks good overall, but who is using the "reload" data?
> 
> ::: devtools/bootstrap.js
> @@ +99,5 @@
> >    }, false);
> >  
> > +  // As we can't get a reference to existing Loader.jsm instances, we send them
> > +  // an observer service notification to unload them.
> > +  Services.obs.notifyObservers(null, "devtools-unload", "reload");
> 
> Who is using this "reload" data?

Loader.jsm: DevToolsLoader.prototype.observe which passes it to `provider.unload(data)`.
It is then passed to unload listeners, but they don't care about this argument.
(devtools.js and devtools-browser.js at end of files: `unload(function () {... })`);
So it is more to respect SDK module loader convention than being really useful.
Attachment #8740932 - Flags: review?(jryans)
Assignee

Updated

3 years ago
Attachment #8739212 - Attachment is obsolete: true
Comment on attachment 8740932 [details] [diff] [review]
Remove unecessary Loader.jsm main in favor of require() - v2

Review of attachment 8740932 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/Loader.jsm
@@ +309,5 @@
>  this.devtools = this.loader = new DevToolsLoader();
>  
>  this.require = this.devtools.require.bind(this.devtools);
> +
> +// For compatiblity reasons, exposes these symbols on "devtools":

Nit: compatibility, expose
Attachment #8740932 - Flags: review?(jryans) → review+
Assignee

Updated

3 years ago
Attachment #8740932 - Attachment is obsolete: true
Assignee

Comment 16

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/cd9c7f0216c4b19d79e607ca7ff04302c6375add
Bug 1261665 - Remove unecessary Loader.jsm main in favor of require(). r=jryans

https://hg.mozilla.org/integration/fx-team/rev/ef461673c13f81ff4bbb162f739227ab75844941
Bug 1261665 - Do not use devtools module in devtools addon before reloading. r=jryans

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd9c7f0216c4
https://hg.mozilla.org/mozilla-central/rev/ef461673c13f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

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