Closed Bug 1392531 Opened 3 years ago Closed 3 years ago

React devtools extension broken in Nightly

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Regression from Bug 1374735.

Stack trace:

> console.error: 
>   Message: Error: Unexpected packet server1.conn2.child1/webExtensionInspectedWindowActor26, {"evalResult":{"value":true},"from":"server1.conn2.child1/webExtensionInspectedWindowActor26"}
>   Stack:
>     onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1309:17
> onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:953:7
> send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:570:13
> exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
> exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
I've been digging into this a bit to figure out which part of Bug 1374735 has introduced the regression and how it has not been caught by the tests.

The reason for the regression is that `DevToolsShim.getTargetForTab` has been introduced by Bug 1374735 (in https://hg.mozilla.org/mozilla-central/rev/e2e779f54e10) as a replacement of `new TabTarget(context.devToolsToolbox.target.tab);` but its implementation reuse the `TabTarget` instance if it already known:

- http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/devtools/client/framework/devtools.js#545-547
- http://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#33-40

This means that different WE contexts (e.g. the devtools page and the devtools panel) are reusing the same "remote debugging protocol" connection, and so two WebExtensionInspectedWindowFront instances created internally for the devtools page and the devtools panel are going to reuse the same connection, and when they would receive the reply for a request created by the other one, the "Unexpected packet" will be raised by the front instance that is not waiting for a response.
Comment on attachment 8899882 [details]
Bug 1392531 - create dedicated tab target instance for web-extensions;

https://reviewboard.mozilla.org/r/171216/#review176342

::: devtools/shim/DevToolsShim.jsm:303
(Diff revision 1)
>   * therefore DevTools should always be available when they are called.
>   */
>  let webExtensionsMethods = [
> +  "createTargetForTab",
>    "createWebExtensionInspectedWindowFront",
>    "getTargetForTab",

You may remove getTargetForTab here and there:
http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools.js#545
Attachment #8899882 - Flags: review?(poirot.alex) → review+
Comment on attachment 8899883 [details]
Bug 1392531 - add test for devtools extensions using inspected window from panel;

https://reviewboard.mozilla.org/r/171218/#review176372

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:246
(Diff revision 1)
> + *
> + * @param {Object} toolbox
> + *        The DevTools toolbox object.
> + * @returns {string} the id of the first additional panel.
> + */
> +function getAdditionPanelId(toolbox) {

Nit, do you mind moving this helper function in the preamble of this test (before the `add_task` calls)?

(In a follow up I'd like to move it into a shared `head_*.js` file to be reused in the other "devtools panel" tests)

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:248
(Diff revision 1)
> + *        The DevTools toolbox object.
> + * @returns {string} the id of the first additional panel.
> + */
> +function getAdditionPanelId(toolbox) {
> +  const toolboxAdditionalTools = toolbox.getAdditionalTools();
> +  const panelDef = toolboxAdditionalTools[0];

I've been thinking about this and, even if there is at least one more test that retrieve the devtools panel this way (by retrieving the first element),
I'm wondering if it would be safer to get it from the end of the array (e.g. using `.pop()`), so that we always get the last added panel (e.g. in case a system addon webextension defines a devtools panel and makes this test to fail, probably a timeout failure).

We could probably also add an assert on the panel title retrieved from the definition, so that if we get the wrong panel for any reason, we get a more clear failure error.

how that sounds to you?
Attachment #8899883 - Flags: review?(lgreco) → review+
Comment on attachment 8899882 [details]
Bug 1392531 - create dedicated tab target instance for web-extensions;

https://reviewboard.mozilla.org/r/171216/#review176376

Thanks for fixing this regression (and for the new related test case)
Attachment #8899882 - Flags: review?(lgreco) → review+
Attachment #8899882 - Attachment is obsolete: true
Comment on attachment 8899917 [details]
Bug 1392531 - create dedicated tab target instance for web-extensions;

Sorry about the r? being triggered again :/ 

I pushed a wrong patch queue by mistake with only the last patch and it seems like mozreview lost track of it.
Attachment #8899917 - Flags: review?(poirot.alex)
Attachment #8899917 - Flags: review?(lgreco)
Attachment #8899917 - Flags: review+
Comment on attachment 8899917 [details]
Bug 1392531 - create dedicated tab target instance for web-extensions;

https://reviewboard.mozilla.org/r/171236/#review176392
Comment on attachment 8899883 [details]
Bug 1392531 - add test for devtools extensions using inspected window from panel;

https://reviewboard.mozilla.org/r/171218/#review176372

Thanks for the review. I won't land until try is green. Feel free to have a 2nd look at what I changed in the meantime.

> Nit, do you mind moving this helper function in the preamble of this test (before the `add_task` calls)?
> 
> (In a follow up I'd like to move it into a shared `head_*.js` file to be reused in the other "devtools panel" tests)

Done

> I've been thinking about this and, even if there is at least one more test that retrieve the devtools panel this way (by retrieving the first element),
> I'm wondering if it would be safer to get it from the end of the array (e.g. using `.pop()`), so that we always get the last added panel (e.g. in case a system addon webextension defines a devtools panel and makes this test to fail, probably a timeout failure).
> 
> We could probably also add an assert on the panel title retrieved from the definition, so that if we get the wrong panel for any reason, we get a more clear failure error.
> 
> how that sounds to you?

Done (at least I hope :) )
(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8899882 [details]
> Bug 1392531 - create dedicated tab target instance for web-extensions;
> 
> https://reviewboard.mozilla.org/r/171216/#review176342
> 
> ::: devtools/shim/DevToolsShim.jsm:303
> (Diff revision 1)
> >   * therefore DevTools should always be available when they are called.
> >   */
> >  let webExtensionsMethods = [
> > +  "createTargetForTab",
> >    "createWebExtensionInspectedWindowFront",
> >    "getTargetForTab",
> 
> You may remove getTargetForTab here and there:
> http://searchfox.org/mozilla-central/source/devtools/client/framework/
> devtools.js#545

getTargetForTab is still used as a helper in some webextensions tests. I will log a follow up to address that separately.
Depends on: 1392737
Thank you for trying! I've just tested the patch, and I'm afraid I'm still getting the same exception, once per second, when react-devtools is installed and enabled.

The stack trace is slightly different now:
> Extension error: Error: Type error for exceptionInfo value (Property "isException" is required) for devtools.inspectedWindow.eval.  0
> [[Exception stack
> Async*createPanelIfReactLoaded@moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:18:25
> setInterval handler*@moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:34:29
> __webpack_require__@moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:9:16
> @moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:14:33
> @moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:1:2
> Current stack
> applySafeWithoutClone@resource://gre/modules/ExtensionCommon.jsm:287:133
> wrapPromise/<@resource://gre/modules/ExtensionCommon.jsm:452:13
> promise callback*wrapPromise@resource://gre/modules/ExtensionCommon.jsm:445:7
> callParentAsyncFunction@resource://gre/modules/ExtensionChild.jsm:837:12
> callAsyncFunction@resource://gre/modules/ExtensionChild.jsm:656:12
> stub@resource://gre/modules/Schemas.jsm:2224:22
> createPanelIfReactLoaded@moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:18:25
> setInterval handler*@moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:34:29
> __webpack_require__@moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:9:16
> @moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:14:33
> @moz-extension://071dca55-9c49-f249-99a4-c240c097c130/build/main.js:1:2
> ]]
(Note that it's not the same as in comment 0, but it's similar to what I reported on github)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/153b72f168d5
create dedicated tab target instance for web-extensions;r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/cb33169e67e1
add test for devtools extensions using inspected window from panel;r=rpl
(In reply to Pulsebot from comment #16)
> Pushed by jdescottes@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/153b72f168d5
> create dedicated tab target instance for web-extensions;r=jdescottes

mozreview edited my commit message and now the r= is wrong :/

Gerald: I can't reproduce the issue so far. 
Did you try this with a clean profile, with only the React DevTools extension installed? 

I am following the STRs below:
- build latest central + patches attached here
- run with new profile 
- install React DevTools 2.5.0 from https://addons.mozilla.org/en-US/firefox/addon/react-devtools/
- open a new tab
- open DevTools

I don't see any error log. I've tried turning on various dump/log prefs, try this on pages with react, without react etc... 
Maybe I'm missing another setting/extension?
https://hg.mozilla.org/mozilla-central/rev/153b72f168d5
https://hg.mozilla.org/mozilla-central/rev/cb33169e67e1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.