1241050 - devtools hot reload addon is weird with gcli commands

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Patrick Brosset <:pbro>]

What I did was:
- started firefox
- made a change to a command (in highlight.js, I just changed the returned output string of the highlight command)
- loaded the devtools addon
- started the dev toolbar
- entered 'highlight div'
=> I got the expected output (corresponding to the change I had just made in highlight.js)

Then:
- changed highlight.js again
- entered 'highlight div' again in the dev toolbar
=> I didn't get the new output

This seemed sort of expected because I hadn't asked the addon to reload from the sources (but, the first time I did the change either).
So I tried:

- opened the toolbox
- hit ctrl+alt+R to reload from sources
- entered 'highlight div' in the dev toolbar again
=> Same result, I didn't get the new output

Then:
- restarted firefox
- entered 'highlight div' in the dev toolbar again
=> This time I got the right output for my command.

The weird thing with the dev toolbar is that it lives outside of the toolbox, whereas the reload functionality lives inside the toolbox. So as a user it's kind of hard to know what to expect.

Comment 1

[Alexandre Poirot [:ochameau]]

Oh wait. The reload shortcut isn't related to the toolbox. You can do it at anytime on any window.
We have a special code to reload the toolbox if we happen to hit the shortcut on a window that has a toolbox already opened.

I see that the gcli toolbar isn't closed when hitting the reload shortcut.
That's a first issue. It should be closed automatically and reopened like the toolbox.
Then if you close and reopen it manually, do you see your changes?

Comment 2

[Alexandre Poirot [:ochameau]]

Looks like it doesn't reload even when closing the toolbar manually.
It is most likely not reloading because of DeveloperToolbar.jsm...

Comment 3

[Alexandre Poirot [:ochameau]]

It looks like this is no usage of this JSM in addons:
  https://mxr.mozilla.org/addons/search?string=DeveloperToolbar.jsm
We should be able to convert it to a module.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Convert DeveloperToolbar.jsm to commonjs module - v1

Just convert it to a module.
It doesn't solve the issue, but allows doing so.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Ensure reloading the developer toolbar - v1

Based on bug 1247270.
Waiting for its review before proceeding.

This is the additional trick to ensure we:
- destroy any existing toolbar
- use the new module instance from browser/ codebase
- open a new toolbar if one was already opened

We would need to strip all devtools specifics from browser/
and instead inject everything dynamically via Addons APIs (WebExtension/Jetpack/Custom helpers).
So that we can easily unregister / re-register stuff within firefox ui on-demand!

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 8717967 [details] [diff] [review]
Convert DeveloperToolbar.jsm to commonjs module - v1

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

Yup. Like it, thanks.

Comment 7

[Alexandre Poirot [:ochameau]]

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

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Cleanup developer-toolbar imports - v1

Now that it is a module, we have access to loader,
we can stop using xpcomutils and cleanup its imports.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Ensure reloading the developer toolbar when using the reload addon.

:jryans just f+ the related code to reload the toolbox and any devtools document living in a tab (about:debugging, jsonview).
Looks like we can more ahead and have a similar reload code for gcli.

This is the critical patch that will make gcli be reloadable via the addon.

Comment 10

[Joe Walker [:jwalker]]

Comment on attachment 8718583 [details] [diff] [review]
Ensure reloading the developer toolbar when using the reload addon.

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

::: devtools/bootstrap.js
@@ +106,5 @@
>          }, false);
>        }
> +
> +      // Manually reload gcli if it has been used
> +      // Bug XXX: Inject the developer toolbar dynamically within browser/

Shouldn't we have a number here?

@@ +108,5 @@
> +
> +      // Manually reload gcli if it has been used
> +      // Bug XXX: Inject the developer toolbar dynamically within browser/
> +      // so that we can easily remove/reinject it
> +      let desc = Object.getOwnPropertyDescriptor(window, "DeveloperToolbar");

const?

@@ +111,5 @@
> +      // so that we can easily remove/reinject it
> +      let desc = Object.getOwnPropertyDescriptor(window, "DeveloperToolbar");
> +      if (desc && !desc.get) {
> +        let wasVisible = window.DeveloperToolbar.visible;
> +        window.DeveloperToolbar.destroy();

It worries me that hide() is async, where destroy() isn't. I think the bug is that if we're doing a destroy() before show() has completed then Bad Things will happen [1].

I'm not sure if we should fix this here. Probably not, unless you think it's likely that this could happen quickly?

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/DeveloperToolbar.jsm#497-520

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #10)
> @@ +111,5 @@
> > +      // so that we can easily remove/reinject it
> > +      let desc = Object.getOwnPropertyDescriptor(window, "DeveloperToolbar");
> > +      if (desc && !desc.get) {
> > +        let wasVisible = window.DeveloperToolbar.visible;
> > +        window.DeveloperToolbar.destroy();
> 
> It worries me that hide() is async, where destroy() isn't. I think the bug
> is that if we're doing a destroy() before show() has completed then Bad
> Things will happen [1].
> 
> I'm not sure if we should fix this here. Probably not, unless you think it's
> likely that this could happen quickly?

Note that we call show() on a completely new instance of DeveloperToolbar. So it can save us from some race on the internal state of DeveloperToolbar.
Otherwhise, do you think that would help if I wait for hide() completetion before calling destroy() and then show() on the second instance?

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Ensure reloading the developer toolbar when using the reload addon - v2

Addressed comments and wait for hide() before calling destroy() and show().

I only see exceptions if I start reloading two times in a row,
which is yet another issue!

Comment 13

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/ea7fde86caa5ed745194d1d45b6b2f0d420f2e8a
Bug 1241050 - Convert DeveloperToolbar.jsm to commonjs module. r=jwalker

https://hg.mozilla.org/integration/fx-team/rev/8bb5aaf8357167ec91a62c4124d8653b5a539b53
Bug 1241050 - Cleanup developer-toolbar imports. r=jwalker

https://hg.mozilla.org/integration/fx-team/rev/34458fbb9cdc7a71babb5fbd73fdd0cfa1d17f4c
Bug 1241050 - Ensure reloading the developer toolbar when using the reload addon. r=jwalker

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea7fde86caa5
https://hg.mozilla.org/mozilla-central/rev/8bb5aaf83571
https://hg.mozilla.org/mozilla-central/rev/34458fbb9cdc