1315251 - Create a DevTools Actor as a backend for the WebExtensions devtools.inspectedWindow API

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

In Bug 1300584 we are going to implement the WebExtensions `devtools.inspectedWindow.eval` and `devtools.inspectedWindow.reload` API methods.

This two methods provides to a devtools page (an invisible context associated to an opened toolbox) or a devtools panel (a new devtools panel registered from a devtools context, usually the devtools page) two features that are similar to the one already provided by the existent DevTools actors (e.g. the webconsole actor has an evaluateJS API method and the tab actor has a reload API method), but they also provides additional features that doesn't seem to fit in the existent devtools actors:

- inspectedWindow.eval has to return a json serialization of the result, instead of an object grip
- inspectedWindow.eval supports optional frameURL, contextSecurityOrigin and useContentScriptContext parameters, which affect where the javascript will be evaluated (in frame with a specified url or origin, or in the content script context of the caller extension)
- inspectedWindow.reload supports an optional injectedScript parameter, which will be evaluated before any other javascript code in all the sub-frames created during the next reload

The goal of this issue is to provide a new DevTools actor which can be used as the backend of the WebExtensions devtools.inspectedWindow API implementation.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

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

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8807559 [details]
Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

https://reviewboard.mozilla.org/r/90710/#review91968

Sorry for the review delay!

Looks good overall.
Here is a bunch of comments which are mostly about code style.

::: devtools/server/actors/webextension-inspected-window.js:26
(Diff revision 1)
> +     */
> +    initialize(conn, tabActor) {
> +      protocol.Actor.prototype.initialize.call(this, conn);
> +      this.tabActor = tabActor;
> +
> +      this.dbg = this.tabActor.makeDebugger();

You should try to create the debugger lazily.

::: devtools/server/actors/webextension-inspected-window.js:30
(Diff revision 1)
> +
> +      this.dbg = this.tabActor.makeDebugger();
> +    },
> +    destroy(conn) {
> +      protocol.Actor.prototype.destroy.call(this, conn);
> +      this.finalize();

What about inlining finilize here?

::: devtools/server/actors/webextension-inspected-window.js:71
(Diff revision 1)
> +                               .getInterface(Ci.nsIDocShell);
> +
> +        if (window == this.window) {
> +          this.customizedReloadWindows.add(window);
> +        } else if (docShell.sameTypeParent) {
> +          let parentWindow = docShell.sameTypeParent

I'm wondering if there can be edge cases where sameTypeParent can be null here?
Like if there is some xbl/xml document loaded in an iframe, or some weird iframe hierarchy, or some random firefox ui document loading in the same time...

::: devtools/server/actors/webextension-inspected-window.js:141
(Diff revision 1)
> +
> +      this.customizedReloadInProgress = false;
> +
> +      this.window.removeEventListener("load", this, true);
> +      Services.obs.removeObserver(this, "document-element-inserted", false);
> +    },

nit: I'm wondering if we could move observe, handleEvent, createCustomizedReloadListener and clearCustomizedReloadListener in a seperate object (in this file, but out of WebExtensionInspectedWindowActor)
So that all these things focusing on the reload are into a coherent class.

::: devtools/server/actors/webextension-inspected-window.js:146
(Diff revision 1)
> +    },
> +
> +    reload(ignoreCache, {userAgent, injectedScript}) {
> +      if (this.isSystemPrincipal(this.window)) {
> +        console.error("inspectedWindow.reload ignored on system principal targets.");
> +        return {};

Shouldn't you return an error like eval?

::: devtools/server/actors/webextension-inspected-window.js:152
(Diff revision 1)
> +      }
> +
> +      if (this.customizedReloadInProgress) {
> +        console.error("inspectedWindow.reload ignored. Reload already in progress.");
> +        return {};
> +      }

I'm just wondering if that's chrome behavior?
I can easily see them piling up the reload requests?
i.e. waiting for the first one to be finished before doing the second?

::: devtools/server/actors/webextension-inspected-window.js:180
(Diff revision 1)
> +          this.clearCustomizedReloadListener();
> +          throw e;
> +        }
> +      };
> +
> +      Services.tm.currentThread.dispatch(delayedReload, 0);

A comment about why we do that would be helpful.

::: devtools/server/actors/webextension-inspected-window.js:185
(Diff revision 1)
> +      Services.tm.currentThread.dispatch(delayedReload, 0);
> +
> +      return {};
> +    },
> +
> +    eval(expression, options, customTargetWindow) {

It would have to comment that customTargetWindow isn't part of the protocol and is only used when the actor code call itself

::: devtools/server/actors/webextension-inspected-window.js:189
(Diff revision 1)
> +
> +    eval(expression, options, customTargetWindow) {
> +      const dbg = this.dbg;
> +      let apiErrorResult;
> +
> +      if (this.isSystemPrincipal(customTargetWindow || this.window)) {

you should first do:
  let window = customTargetwindow || this.window;
and then use window.

::: devtools/server/actors/webextension-inspected-window.js:195
(Diff revision 1)
> +        apiErrorResult = {
> +          code: "E_PROTOCOLERROR",
> +          description: "Inspector protocol error: %s",
> +          details: [
> +            "This target has a system principal. inspectedWindow.eval denied.",
> +          ],

Actors are using following format to notify about error:
return {
  error: "protocolError",
  message: "inspectedWindow.eval denied on document having a system prinvipal"
};
Then the client knows when there is an error by checking if the response object has an error attribute.

Do you really need 3 attributes?

::: devtools/server/actors/webextension-inspected-window.js:198
(Diff revision 1)
> +          details: [
> +            "This target has a system principal. inspectedWindow.eval denied.",
> +          ],
> +        };
> +
> +        return {apiErrorResult};

nit: no need ob apiErrorResult intermediate variable here.

::: devtools/server/actors/webextension-inspected-window.js:241
(Diff revision 1)
> +            lineNumber,
> +            columnNumber,
> +          };
> +        }
> +      } else {
> +        // TODO: send error on no result

Is that really an error? does that mean that executeInGlobalWithBinding failed or that the expression just doesn't return a value?

::: devtools/server/actors/webextension-inspected-window.js:259
(Diff revision 1)
> +            code: "E_PROTOCOLERROR",
> +            description: "Inspector protocol error: %s",
> +            details: [
> +              String(err),
> +            ],
> +          };

I think you should just return the error object.
I'm not sure there is a lot of value in returning a undefined evalResult and evalExceptionResult.

::: devtools/server/tests/browser/browser_webextension_inspected_window.js:12
(Diff revision 1)
> +const {
> +  WebExtensionInspectedWindowFront
> +} = require("devtools/shared/fronts/webextension-inspected-window");
> +
> +function* setup(pageUrl, isAbsoluteURL=false) {
> +  yield addTab(`${isAbsoluteURL ? "" : MAIN_DOMAIN}${pageUrl}`);

Wouldn't it be just clearer/easier by letting the callsite always pass absolute url
  yield setup(MAIN_DOMAIN);
  yield setup("about:addons");
?

::: devtools/server/tests/browser/browser_webextension_inspected_window.js:20
(Diff revision 1)
> +  const client = new DebuggerClient(DebuggerServer.connectPipe());
> +  const form = yield connectDebuggerClient(client);
> +
> +  const tabClient = yield new Promise(resolve => {
> +    client.attachTab(form.actor, (res, tabClient) => resolve(tabClient));
> +  });

let [, tabClient] = yield client.attachTab(form.actor);

::: devtools/server/tests/browser/browser_webextension_inspected_window.js:24
(Diff revision 1)
> +    client.attachTab(form.actor, (res, tabClient) => resolve(tabClient));
> +  });
> +
> +  const consoleClient = yield new Promise(resolve => {
> +    client.attachConsole(form.consoleActor, [], (res, consoleClient) => resolve(consoleClient));
> +  });

I imagine we can do the same with attachConsole:
let [, consoleClient] = client.attachConsole(form.consoleActor, []);

::: devtools/server/tests/browser/browser_webextension_inspected_window.js:123
(Diff revision 1)
> +  const waitForNextTabNavigated = () => {
> +    return new Promise(resolve => {
> +      client.addListener("tabNavigated", function (evt, pkt) {
> +        if (pkt.state == "stop" && pkt.isFrameSwitching == false) {
> +          resolve();
> +        }

You should also call removeListener before resolving.

::: devtools/server/tests/browser/browser_webextension_inspected_window.js:172
(Diff revision 1)
> +        if (pkt.state == "stop" && pkt.isFrameSwitching == false) {
> +          resolve();
> +        }
> +      });
> +    });
> +  };

You should share waitForNextTabNavigated by accepting a client argument.

::: devtools/server/tests/browser/browser_webextension_inspected_window.js:220
(Diff revision 1)
> +        }
> +      });
> +    });
> +  };
> +
> +  // Test reload with custom userAgent.

This comment looks wrong/outdated/copy pasted?

::: devtools/server/tests/browser/inspectedwindow-reload-target.sjs:48
(Diff revision 1)
> +
> +  const frames = parseInt(params.get("frames"));
> +  let content = "";
> +
> +  if (frames > 0) {
> +    content = `<iframe seamless src="?test=injected-script&frames=${frames - 1}"></iframe>`;

Is seamless any related to this patch??

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1315251 - Fix excluded 'devtools/server/tests' in .eslintignore.

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

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8807559 [details]
Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

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

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8810119 [details]
Bug 1315251 - Fix excluded 'devtools/server/tests' in .eslintignore.

https://reviewboard.mozilla.org/r/92520/#review92652

::: .eslintignore:129
(Diff revision 1)
> -devtools/server/tests/**
> +devtools/server/tests/browser/**
> +devtools/server/tests/mochitest/**
> +devtools/server/tests/unit/**

I noticed that the test file added in the other patch from this issue was skipped by eslint, and when I tried to exclude it from the eslint ignore list, I noticed that the `devtools/server/tests/**` glob pattern prevents the single test from being linted.

This patch fix the eslint ignore pattern related to the `devtools/server/tests` sub-dirs by exploding it into 3 separated ignore patterns.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8807559 [details]
Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

https://reviewboard.mozilla.org/r/90710/#review91968

> I'm wondering if there can be edge cases where sameTypeParent can be null here?
> Like if there is some xbl/xml document loaded in an iframe, or some weird iframe hierarchy, or some random firefox ui document loading in the same time...

My goal is handling this scenarios as follows:

- prevent to execute `devtools.inspectedWindow.reload` on any "non web-content" target
- during the customized page reload, filter out any sub-frame that is not a "web-content" target (by checking that its `sameTypeParent` is not null and that it is already a tracked window)

> nit: I'm wondering if we could move observe, handleEvent, createCustomizedReloadListener and clearCustomizedReloadListener in a seperate object (in this file, but out of WebExtensionInspectedWindowActor)
> So that all these things focusing on the reload are into a coherent class.

This sounds definitely like the right thing to do here, I refactored out this part of the actor in a new CustomizedReload class, which is instantiated from the actor when a customized reload is needed.

> Shouldn't you return an error like eval?

Unfortunately the `devtools.inspectedWindow.reload` method doesn't support any callback to which we can report this error asynchronously, and so there will be no-one to listen to the return value (that is the reason why I opted to log an error)

On the client side, the WebExtensions API implementation side, I'm going to double this check and synchronously raise an exception when the toolbox target is a local tab that does not contain a regular webpage.

> I'm just wondering if that's chrome behavior?
> I can easily see them piling up the reload requests?
> i.e. waiting for the first one to be finished before doing the second?

I didn't checked what Chrome does here, but I preferred to prevent the reload requests to pile up because I'm not sure if it is the right way to handle this scenario.

The reason is that I'm assuming that by piling the reload requests, a devtools addon can flood Firefox of an high number of reload requests and the browser instance can become unresponsive as a result, and I would like to prevent a devtools addon from breaking the browser at that level if possible.

> Actors are using following format to notify about error:
> return {
>   error: "protocolError",
>   message: "inspectedWindow.eval denied on document having a system prinvipal"
> };
> Then the client knows when there is an error by checking if the response object has an error attribute.
> 
> Do you really need 3 attributes?

The reason of this particular format for the eval errors and exceptions is that it is the format defined in the API schemas, and even if none of the existent devtools addons should make assumptions on the exception and error messages content, I think that we have to keep the format of the error object consistent to the one declared in the JSON schema.

To make more clear that this exceptions and errors are part of the actor result format, I've changed the actor specs to be more explicit (and I added inline comments above their definitions, to provide more context informations near to them)

One thing that I didn't do yet, and I'm going to handle as part of the next update of the WebExtensions API implementation patches, is to check for errors received on the client side in the error format commonly used by our RDP protocol and report them as errors in the JSON schema format that the extension code expects.

> Is that really an error? does that mean that executeInGlobalWithBinding failed or that the expression just doesn't return a value?

By looking at other existent usages of `executeInGlobalWithBinding` and its docs in "js/src/doc/Debugger/", I'm not sure that this is something that is going to happen (it would mean that the result is not any of a return, a yield or a throw).

Currently I opted to log it as an error, so that if it happens, at least it will produce an error log.

> Is seamless any related to this patch??

I added the `seamless` attribute on the generated iframes so that, if the test fails, it is more likely that the content of all the nested iframes are going to be fully visible in the failure screenshot, and make it easier to understand what was going on.

I forgot to put the reason in a comment in the ".sjs" file, I added an inline comment in the updated version of this patch.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Besides the changes highlighted in the above comments, in the updated patches I added the following changes, that definitely worth an explicit mention:

- actor specs: added new named dict types for the data received and returned by this actor, instead of using the generic json type (the goal of this change is to make the format of the actor inputs/outputs more explicit and documented).

- the new `callerInfo` param: I added a new `webExtensionCallerInfo` dict type, which contains the addonId, an url, and optionally a lineNumber of the caller, which is used to include in the logged error messages enough information to make it clear which is the addon (and possibly which part of the addon) is generating the error (and in follow ups, the addonId will be needed to select the correct content script context once we are going to implement the `useContentScriptContext` eval param, and to make the eval able to inject javascript code in tab pages which are provided by the same devtools addon).

- the new CustomReload is now detecting if the page reload has been stopped (by listening to the "stop" event while the target is still loading), I'm currently also invalidating the custom reload if the page receive a "beforeunload" event while the target is still loading but I'm not completely sure about this (I'm thinking that we should probably follow the navigation until the final page load has been received, I'm going to create two new test cases for the "stop" and "beforeunload" events, and try this scenarios on chrome and check how it behaves in these scenarios).

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8807559 [details]
Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

https://reviewboard.mozilla.org/r/90710/#review93078

Looks good, thanks Luca!

::: devtools/server/actors/webextension-inspected-window.js:73
(Diff revision 2)
> +
> +    return this.waitForReloadCompleted;
> +  },
> +
> +  observe(subject, topic, data) {
> +    if (topic == "document-element-inserted") {

nit: to save up some indentation, I often do:
if (topic != "document-element-inserted") {
  return;
}

::: devtools/server/actors/webextension-inspected-window.js:97
(Diff revision 2)
> +          this.customizedReloadWindows.add(window);
> +        }
> +      }
> +
> +      // If the window has been to the customizedReloadWindows set,
> +      // inject the script before any other page script has been executed.

This comment isn't helpful. It comments the code we can already read and understand.
You should translate "window has been to the customizedReloadWindows set" to something that explains what does it mean when a window is in the set.

::: devtools/server/actors/webextension-inspected-window.js:122
(Diff revision 2)
> +        // and cancel the reload on the stop and beforeunload events.
> +        window.addEventListener("load", this, true);
> +        window.addEventListener("stop", this, true);
> +        // TODO(rpl): check how chrome behaves when the page navigate to another url
> +        // during the reload.
> +        window.addEventListener("beforeunload", this, true);

Any particular reason to use beforeunload instead of unload?
beforeunload looks possibly wrong if a returnValue is set, a prompt is shown and the user says no. Then I imagine the reload will achieve.
But honestly that looks like a very rare scenario.
A brief comment about why you originaly choose beforeunload would be enough.

::: devtools/server/actors/webextension-inspected-window.js:223
(Diff revision 2)
> +    },
> +
> +    /**
> +     * Reload the target tab, optionally customize the cache bypass,
> +     * the userAgent and/or inject a script in every sub-frame until the reload
> +     * has been completed.

I would rather phrase it like this:
"Reload the target tab, optionally bypass cache, customize the userAgent and/or inject a script in targeted document or any of its sub-frame."

::: devtools/server/actors/webextension-inspected-window.js:289
(Diff revision 2)
> +            if (ignoreCache) {
> +              reloadFlags |= Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;
> +            }
> +            this.webNavigation.reload(reloadFlags);
> +          }
> +        } catch (err) {

I would move that try/catch within the `if (injectedScript || userAgent) {` block.

::: devtools/server/actors/webextension-inspected-window.js:311
(Diff revision 2)
> +    },
> +
> +    /**
> +     * Evaluate the provided javascript code in the target window
> +     * the userAgent and/or inject a script in every sub-frame until the reload
> +     * has been completed.

Is this comment outdated?
I don't see why we task about user agent, sub frames and reload here.
This method is only about injecting a script in one document.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8810119 [details]
Bug 1315251 - Fix excluded 'devtools/server/tests' in .eslintignore.

https://reviewboard.mozilla.org/r/92520/#review93138

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8810119 [details]
Bug 1315251 - Fix excluded 'devtools/server/tests' in .eslintignore.

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

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8807559 [details]
Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

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

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8807559 [details]
Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

https://reviewboard.mozilla.org/r/90710/#review93078

> Any particular reason to use beforeunload instead of unload?
> beforeunload looks possibly wrong if a returnValue is set, a prompt is shown and the user says no. Then I imagine the reload will achieve.
> But honestly that looks like a very rare scenario.
> A brief comment about why you originaly choose beforeunload would be enough.

There was no particular reason to prefer the "beforeunload" to the "unload" event, and so I created a new test case for the "interrupted customized reload" scenario and I tested both successfully (but then, as I'm going to describe in a later comment, I changed CustomizedReload to use nsIWebProgressListener instead of the DOM events).

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8807559 [details]
Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

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

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Besides the changes highlighted in the above comments, in the updated patches I added the following changes, that are big enough to worth an explicit mention:

- in CustomizedReload, this.window is not anymore a getter that returns this.tabActor.window, the reason is that I thought that if the tabActor is switched to a sub-frame during the reload (due to a "toolbox frame switching" requested by the user during a slow page reload), then we want to be sure that this.window points to the same window that the CustomizedReload instance was targeting when has been started.

- CustomizedReload is now a nsIWebProgressListener instead of a DOM event listener, mostly because I think that it is more likely to cover some additional "interrupted reload" scenarios, and it is how most of the other features that tracks the page reloading are currently doing it (e.g. the
webNavigation API uses it for similar reasons).

- CustomizedReload tracks the window where an injectedScript is injected using a WeakSet instead of a Set (given that we are not iterating over this set, it is better to keep a weak reference for them)

- CustomizedReload observes the document-element-inserted notification event only if there's an injectedScript to evaluate.

Alex,
How the changes described above look to you?
(especially the "nsIWebProgressListener" change)

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8807559 [details]
Bug 1315251 - Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90710/diff/4-5/

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

After a brief discussion over IRC with Alex, we agreed to take a a look if we can use the existent tabActor events in the CustomizedReload helper class (e.g. navigate and will-navigate, which are emitted by the tabActor from a nsIWebProgressListener, similarly to how the last version of the CustomizedReload helper class is detecting if the current reload has been completed).

Unfortunately, after looking deeper into how using the tabActor events would change the CustomizedReload class (e.g. when these events are emitted and what  informations are available in the event handler once the event has been received), I'm not fully convinced that this change would make it simpler, e.g.:

- will-navigate and navigate events can be related to the "toolbox frame switching" (when the user change the target frame for the toolbox) instead of an actual page reload and currently we cannot detect it from the listener without making changes to these TabActor events, because this information is not currently sent in the data object related to the emitted will-navigate/navigate events

- tabActor seems to always emit will-navigate before the navigate event when the target tab is starting to reload to notify that the current page is being unloaded (but when the page is starting to reload we do not need to do anything yet in CustomizedReload, which means that we have to detect when the will-navigate is related to an interrupted reload and when it is related to the reload that we just started)

- we need to subscribe two event listeners instead of one nsIWebProgressListener (and we have to manually bind the listeners methods to the current CustomizedReload instance to be sure that `this` is CustomizedReload instance when our event listeners are called)

Another reason why I still prefer the current approach is that the navigate and will-navigate events behavior is highly connected to how the toolbox works (e.g. the frame switching scenario briefly described above) and I'm a bit worried that at some point the needs to the toolbox can conflict even more with the needs of this particular API method (which sometimes can require changes which are not relevant or totally unwanted from a toolbox point of view, e.g. some required behavior related to Chrome extensions compatibility).

What do you think Alex? 
are the behaviors described above correctly described (or am I missing/misreading something)?
do you think that we should try to adapt the CustomReload to use the tabActor events given the concerns described above?

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to Luca Greco [:rpl] from comment #16)
> After a brief discussion over IRC with Alex, we agreed to take a a look if
> we can use the existent tabActor events in the CustomizedReload helper class
> (e.g. navigate and will-navigate, which are emitted by the tabActor from a
> nsIWebProgressListener, similarly to how the last version of the
> CustomizedReload helper class is detecting if the current reload has been
> completed).
> 
> Unfortunately, after looking deeper into how using the tabActor events would
> change the CustomizedReload class (e.g. when these events are emitted and
> what  informations are available in the event handler once the event has
> been received), I'm not fully convinced that this change would make it
> simpler, e.g.:
> 
> - will-navigate and navigate events can be related to the "toolbox frame
> switching" (when the user change the target frame for the toolbox) instead
> of an actual page reload and currently we cannot detect it from the listener
> without making changes to these TabActor events, because this information is
> not currently sent in the data object related to the emitted
> will-navigate/navigate events

This is not the main purpose of these events. Above all they are meant to notify about actual document navigations. We can always pass "isIframeSwitching" into these events if that helps.

> - tabActor seems to always emit will-navigate before the navigate event when
> the target tab is starting to reload to notify that the current page is
> being unloaded (but when the page is starting to reload we do not need to do
> anything yet in CustomizedReload, which means that we have to detect when
> the will-navigate is related to an interrupted reload and when it is related
> to the reload that we just started)

?
I don't see why you would listen to will-navigate. navigate looks like what you want to replace your onStateChange function. 

> - we need to subscribe two event listeners instead of one
> nsIWebProgressListener (and we have to manually bind the listeners methods
> to the current CustomizedReload instance to be sure that `this` is
> CustomizedReload instance when our event listeners are called)

This is not really important.

> Another reason why I still prefer the current approach is that the navigate
> and will-navigate events behavior is highly connected to how the toolbox
> works (e.g. the frame switching scenario briefly described above) and I'm a
> bit worried that at some point the needs to the toolbox can conflict even
> more with the needs of this particular API method (which sometimes can
> require changes which are not relevant or totally unwanted from a toolbox
> point of view, e.g. some required behavior related to Chrome extensions
> compatibility).

That's the critical thing here. Would you at some point need something drastically different?
As of today, it looks like your document-element-inserted listener could be replace by a window-ready listener and onStateChange by navigate. Both would check for event.isTopLevel && event.isIframeSwitching. Note that your sameParentType trick would be replaced by a simple check to event.isTopLevel.

> do you think that we should try to adapt the CustomReload to use the
> tabActor events given the concerns described above?

I already r+ your patches so I was ok with you reimplementing these listeners as-is.
I would suggest you to switch to these events if you start reimplementing something already fixed/implemented by them.

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

One more push to try:

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

it looks good (there are just a bunch of unrelated oranges)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e7f1ee0d65b
Fix excluded 'devtools/server/tests' in .eslintignore. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/5d0207006a7e
Create a DevTools Remote Debugger Actor as a backend for the WebExtension DevTools API. r=ochameau

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e7f1ee0d65b
https://hg.mozilla.org/mozilla-central/rev/5d0207006a7e