Closed Bug 1435959 Opened 6 years ago Closed 6 years ago

Network monitor for add-on background script doesn't work on Windows

Categories

(WebExtensions :: Developer Tools, defect, P2)

59 Branch
defect

Tracking

(firefox58 wontfix, firefox59 wontfix, firefox60 verified)

VERIFIED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- verified

People

(Reporter: juraj.masiar, Assigned: rpl)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180201171410

Steps to reproduce:

Using Windows 10 and Firefox 58, 59 (Beta or Developer edition) or Alpha 60.

1) install add-on that is making remote requests in background script
2) debug background script using Developer tools and open Netwrok tab
3) make some remote requests in add-on


Actual results:

No requests are being logged.


Expected results:

Requests should be visible.

This works on Ubuntu using Release version 58.
It also works for normal pages, only add-ons seems to be affected.
I'm actually seeing the same/similar issue on a Mac (High Sierra).
I have a call to an api going out in the background script and it's obviously succeeding because I'm getting the data I need from it, but the request is never logged in the add-on debugger's Network tab.
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180205211730

Hello,

I have tested this issue on latest Firefox Developer Edition 59.0b7, latest Firefox release 58.0.2 and could not reproduce it. I have installed Min Vid add-on from https://testpilot.firefox.com/experiments/min-vid,
navigated to www.youtube.com, sent a song to Min Vid player using the "Play now" option, opened Developer Tools -> Network tab and the requests were sent (GET and POST) and visible.

Can you please name the addon which you are using or another one, where we can observe the issue? 

Also, can you please retest this on an older Firefox version, such as 57.0 and report back the results? In this way, we'll see if this issue is a regression. When doing this, please use a new clean Firefox profile (https://goo.gl/AWo6h8), maybe even safe mode (https://goo.gl/AR5o9d), to eliminate custom settings as a possible cause.
Flags: needinfo?(juraj.masiar)
Video of the bug in Windows using clean profile Firefox 59 developer edition and then working example in Ubuntu using Firefox 58
Flags: needinfo?(juraj.masiar)
I'm using GroupSpeedDial add-on:
https://addons.mozilla.org/en-US/firefox/addon/groupspeeddial/
It makes a request when you click the "Login / Create account" label.
Thanks for the add-on. I've also managed to reproduce the issue using the provided add-on, on latest Firefox Developer Edition 59.0b7 and latest Firefox release 58.0.2: when I clicked the "Login/Create an account" section from the doorhanger no request was sent in "Network" tab from the debug background script.

On latest Nightly build 60.0a1 (2018-02-09), the "Incoming Connection" dialog wasn't displayed, but still there were no requests displayed in the "Network" tab.

Indeed, this issue is not reproducible on Ubuntu 14.04 x64, using latest Firefox release 58.0.2.

I think the suitable component for this is: Firefox:: Developer Tools: Console.
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Console
Ever confirmed: true
Component: Developer Tools: Console → WebExtensions: Developer Tools
Product: Firefox → Toolkit
Flags: needinfo?(lgreco)
Is "extensions.webextensions.remote" relevant? (Currently it is only enabled on Windows.)
(In reply to Masatoshi Kimura [:emk] from comment #6)
> Is "extensions.webextensions.remote" relevant? (Currently it is only enabled
> on Windows.)

Yes, it is. Basically when "extensions.webextensions.remote" is true the extension runs in its own extension process,
on the contrary if it is a false the extension runs in the main browser process.

I've been able to reproduce the issue by creating a small extension with a background page which calls `fetch` to create a network request from the background extension page, and when "extensions.webextensions.remote" is true the request is not caught by the network monitor and then it is not visible in the network panel.

The main reason for the missing network events seems to be due to the webconsole actor which is currently not enabling the network monitor in an oop compatible mode (as we do for a tab that is running in the child process) for a "WebExtensionChildActor" actor:

- https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/devtools/server/actors/webconsole.js#590-592,634-643
Flags: needinfo?(lgreco)
Assignee: nobody → lgreco
Blocks: webext-oop
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

https://reviewboard.mozilla.org/r/221472/#review227256


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/webconsole.js:89
(Diff revision 1)
>      Services.obs.addObserver(this._onObserverNotification,
>                               "last-pb-context-exited");
>    }
>  
>    this.traits = {
> -    customNetworkRequest: !this._parentIsContentActor,
> +    customNetworkRequest: !this._parentIsContentActor && !this._parentIsWebExtensionChildActor,

Error: Line 89 exceeds the maximum line length of 90. [eslint: max-len]
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

Hi Alex and Jan,
I'd like to get a feedback from you on the proposed set of changes from this patch, applied to the devtools internals to fix the netmonitor behavior for the extensions that are running in the extension process, and the new test case.

I'm still looking into how it would be better to test this proposed set of changes (I've currently included in this patch a new test file in the WebExtensions mochitest-browser dir browser/components/extensions/test/browser/ because in this directory there is already in place a strategy to ensure that we run the tests with both the extensions oop mode enabled and disabled, I'm not super-happy to have this test here instead of the netmonitor test dir, but it allowed me to immediately prepare a new test case to verify the fix, while we can discuss and evaluate where we would like to have these "addon debugging" tests so that they are executed in both the oop mode).
Attachment #8952217 - Flags: feedback?(poirot.alex)
Attachment #8952217 - Flags: feedback?(odvarko)
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

I was curious about this patch as I'm thinking about ways to rework Network Monitor actors in the future.  I think the core change here look reasonable.

As for the test, I think it seems okay where it is for now, since like you said, this directory has the infrastructure to run in both process modes.  

As a follow up, let's add that same infrastructure to some DevTools folder and move add-on debugging tests like this.  One option could be a new subfolder for add-on tests inside devtools/client/framework/test, since the framework includes the pieces to launch the special toolbox used for add-on debugging, and it also tests other "exotic" toolboxes like Browser Toolbox.  Another possibility could be a subfolder in devtools/client/aboutdebugging/test, since that's where you launch add-on debugging currently.  (Though, I have grand plans to make more add-on bits accessible from other kinds of toolboxes, so I don't view it as an about:debugging only feature.)
Attachment #8952217 - Flags: feedback+
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

I agree that having the test being part of the WebExtensions test suite is ok and currently the way to go. As soon as we have similar infrastructure for DevTools test framework we can put further tests directly into DevTools dir.
Thanks for working on this Luca!
Honza
Attachment #8952217 - Flags: feedback?(odvarko) → feedback+
(In reply to Luca Greco [:rpl] from comment #11)
> I've currently included in this patch a new test file in the
> WebExtensions mochitest-browser dir
> browser/components/extensions/test/browser/ because in this directory there
> is already in place a strategy to ensure that we run the tests with both the
> extensions oop mode enabled and disabled

We already have a lot of devtools tests for all WebExt API related to devtools in this folder.
I'm not sure there is an ideal spot for them, we will always hesitate between the two locations.
Now, this test isn't specific to a WebExt API, so it makes it just slightly better located in /devtools/.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> As a follow up, let's add that same infrastructure to some DevTools folder
> and move add-on debugging tests like this.  One option could be a new
> subfolder for add-on tests inside devtools/client/framework/test, since the
> framework includes the pieces to launch the special toolbox used for add-on
> debugging, and it also tests other "exotic" toolboxes like Browser Toolbox. 
> Another possibility could be a subfolder in
> devtools/client/aboutdebugging/test, since that's where you launch add-on
> debugging currently.  (Though, I have grand plans to make more add-on bits
> accessible from other kinds of toolboxes, so I don't view it as an
> about:debugging only feature.)

I have a more precise picture because of my experience on luciddream.
We shouldn't have a folder or test per target kind.
Instead we should have a couple of smoketests, that we could run against all exotic targets.
So that you should be able to write the test once and the test framework will get it executed against all targets/environments.

WebExt choosed to have have them run multiple times via duplicated browser.ini:
  https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser-remote.ini
  https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser.ini
With a particular trick in browser-remote.ini:
  install-to-subdir = test-oop-extensions
In order to help head.js differenciate the two executions:
  let remote = gTestPath.includes("test-oop-extensions");

After that, it looks like, the only difference between the two runs is that:
  https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/head.js#45-57
  SpecialPowers.pushPrefEnv({set: [
    ["extensions.webextensions.remote", remote],
  ]});
  if (remote) {
    // We don't want to reset this at the end of the test, so that we don't have
    // to spawn a new extension child process for each test unit.
    SpecialPowers.setIntPref("dom.ipc.keepProcessesAlive.extension", 1);
  }
Could you confirm that Luca?

I imagine we could easily redo that from devtools folder without immediately introducing multiple browser.ini files.
(execute add_task function twice with the two pref values)

Regarding DevTools needs, I would move yet another layer of the test to the test framework.
In my mind, the test script would only be `toolboxProcessScript`, all the rest will be managed by a shared test helper. The test harness will setup all what is necessary for each "exotic target" (addon, oop addon, browser toolbox target, worker, ...) and then we will execute the same assertions against each of these exotic targets. Otherwise, we will most likely have to copy paste test assertions similar to `toolboxProcessScript` to cover all basic devtools features against various targets.
Note that we can also do the other way around. Have `toolboxProcessScript` be the shared helper if that's any easier to maintain/understand.
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

https://reviewboard.mozilla.org/r/221472/#review227760

Looks good to me.

I'm not so concerned about the folders, nor testing about many exotic targets. At least not immediately.
I'm much more concerned about MOZ_TOOLBOX_TEST_SCRIPT and its limitations (string evaluations, no assertion helpers, no access to head.js or anything).
It forced you to do surprising things to use WebExt assertion API, I think this is a sign we should improve browser toolbox test framework sooner than later!

::: browser/components/extensions/test/browser/browser_ext_addon_debugging_netmonitor.js:87
(Diff revision 2)
> +    });
> +
> +    let {store} = netmonitor.panelWin;
> +
> +    let requests = Array.from(store.getState().requests.requests.values());
> +    await jsterm.execute(`testNetworkRequestReceived(${JSON.stringify(requests)});`);

These two lines aren't obvious.
Could you add a comment to say that's because browser toolbox test script don't have assertions API and so you execute an addon method that itself has assertion helpers available?

::: devtools/server/actors/webconsole.js:89
(Diff revision 2)
>      Services.obs.addObserver(this._onObserverNotification,
>                               "last-pb-context-exited");
>    }
>  
>    this.traits = {
> -    customNetworkRequest: !this._parentIsContentActor,
> +    customNetworkRequest: !this._parentIsContentActor &&

I'm wondering if we could have a more generic check here:
* by having ContentActor and WebExtensionChildActor both inherit from a common class? (Seems unlikely?) Which main goal would be to expose a messageManager
* checking if we are in child process and assume that all actors instanciated in child process will support this? (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) May be fragile as that doesn't enfore parentActor.messageManager to be defined.

Otherwise, here and from onStartListeners, instead of introducing `_parentIsWebExtensionChildActor`, I would rename `_parentIsContentActor` to `supportsNetworkMonitor` and have all the checks being done from here. May be that's enough to make things clear.
Attachment #8952217 - Flags: review+
Attachment #8952217 - Flags: feedback?(poirot.alex) → feedback+
(In reply to Alexandre Poirot [:ochameau] from comment #14)
> I have a more precise picture because of my experience on luciddream.
> We shouldn't have a folder or test per target kind.
> Instead we should have a couple of smoketests, that we could run against all
> exotic targets.
> So that you should be able to write the test once and the test framework
> will get it executed against all targets/environments.
> 
> WebExt choosed to have have them run multiple times via duplicated
> browser.ini:
>  
> https://searchfox.org/mozilla-central/source/browser/components/extensions/
> test/browser/browser-remote.ini
>  
> https://searchfox.org/mozilla-central/source/browser/components/extensions/
> test/browser/browser.ini
> With a particular trick in browser-remote.ini:
>   install-to-subdir = test-oop-extensions
> In order to help head.js differenciate the two executions:
>   let remote = gTestPath.includes("test-oop-extensions");
> 
> After that, it looks like, the only difference between the two runs is that:
>  
> https://searchfox.org/mozilla-central/source/browser/components/extensions/
> test/browser/head.js#45-57
>   SpecialPowers.pushPrefEnv({set: [
>     ["extensions.webextensions.remote", remote],
>   ]});
>   if (remote) {
>     // We don't want to reset this at the end of the test, so that we don't
> have
>     // to spawn a new extension child process for each test unit.
>     SpecialPowers.setIntPref("dom.ipc.keepProcessesAlive.extension", 1);
>   }
> Could you confirm that Luca?

Yes, I confirm that (I also applied the same strategy to the devtools/client/aboutdebugging/test 
directory locally while working on other patches related to changes to the devtools internals
that had to be tested in both oop and non oop extension mode, but I wasn't fully convinced
that it was the right place to do it and so I opted for a simpler strategy,
https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/devtools/server/tests/mochitest/test_webextension-addon-debugging-connect.html
, that was enough for that case and discuss a more complete strategy with the DevTools team)
 
> I imagine we could easily redo that from devtools folder without immediately
> introducing multiple browser.ini files.
> (execute add_task function twice with the two pref values)

Yeah, this is basically the strategy that we used for
https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/devtools/server/tests/mochitest/test_webextension-addon-debugging-connect.html

> Regarding DevTools needs, I would move yet another layer of the test to the
> test framework.
> In my mind, the test script would only be `toolboxProcessScript`, all the
> rest will be managed by a shared test helper. The test harness will setup
> all what is necessary for each "exotic target" (addon, oop addon, browser
> toolbox target, worker, ...) and then we will execute the same assertions
> against each of these exotic targets. Otherwise, we will most likely have to
> copy paste test assertions similar to `toolboxProcessScript` to cover all
> basic devtools features against various targets.
> Note that we can also do the other way around. Have `toolboxProcessScript`
> be the shared helper if that's any easier to maintain/understand.

I definitely agree with you that improving the way we test what happens in
the browser toolbox process is definitely needed (as you also pointed out in 
comment 15).

In the similar tests from devtools/client/aboutdebugging we are using the
console API messages to coordinate the part of the test that runs in the
toolbox process with the one that runs in the Firefox process, which seems
a crude hack, and in the test part of the attached patch I'm using the
webconsole to execute functions defined in the target extension to be able
to both coordinate the test in the two processes and make some assertions
on the results, which is a pretty similar crude hack (and it is still the
confusing part of the test).

And so +1 from me on a follow up to provide a better testing strategy
that we can share between the different "exotic targets" that share
these needs.
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

https://reviewboard.mozilla.org/r/221472/#review227760

> These two lines aren't obvious.
> Could you add a comment to say that's because browser toolbox test script don't have assertions API and so you execute an addon method that itself has assertion helpers available?

I totally agree, comment added.

> I'm wondering if we could have a more generic check here:
> * by having ContentActor and WebExtensionChildActor both inherit from a common class? (Seems unlikely?) Which main goal would be to expose a messageManager
> * checking if we are in child process and assume that all actors instanciated in child process will support this? (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) May be fragile as that doesn't enfore parentActor.messageManager to be defined.
> 
> Otherwise, here and from onStartListeners, instead of introducing `_parentIsWebExtensionChildActor`, I would rename `_parentIsContentActor` to `supportsNetworkMonitor` and have all the checks being done from here. May be that's enough to make things clear.

I changed the patch to try to retrieve the message manager if there is `processBoundary` is true, instead of checking the class name of the parent actor, but I also had to add a check if Services.appinfo was actually defined because it was failing on a worker test (because it seems that Services.appinfo was undefined there).

I'm not sure about the customNetworkRequest, I tried to find what it is its role by seaching in the rest of the devtools internals but I've not yet found where and how it is used, and so I have not changed it yet.
(and I have not yet defined a `supportsNetworkMonitor` getter, because I'm not fully convinced that it would be the right name for an unified getter, mainly because the network monitor is basically supported even when the actor is not running in a child process, and so it I've not applied this change yet neither)
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

https://reviewboard.mozilla.org/r/221472/#review230688

::: browser/components/extensions/test/browser/browser_ext_addon_debugging_netmonitor.js:94
(Diff revision 3)
> +    // NOTE: we need to filter the requests to the ones that we expect until
> +    // the network monitor is not yet filtering out the requests that are not
> +    // coming from an extension window or a descendent of an extension window,
> +    // in both oop and non-oop extension mode (filed as Bug 1442621).
> +    function filterRequest(request) {
> +      return request.url === expectedURL;
> +    }
> +
> +    let requests;
> +
> +    await waitFor(() => {
> +      requests = Array.from(store.getState().requests.requests.values())
> +                      .filter(filterRequest);
> +
> +      return requests.length > 0;
> +    });
> +
> +    // Call a function defined in the target extension to make assertions
> +    // on the network requests collected by the netmonitor panel.
> +    await jsterm.execute(`testNetworkRequestReceived(${JSON.stringify(requests)});`);

I added an additional change to the test case, which filters the network requests collected by the netmonitor panel to only check the ones that we are interesting in, because it doesn't yet filter out the other unrelated requests (which is something that we have to fix for both oop and non-oop extensions) and it was making this test to fail intermittently on TV (test-verify).

I took a look at the additional changes needed to also filter out the network requests that are not originated from the target extension pages (and their descendant frames), but I opted to file this issue as a follow up (Bug 1442621, also added in the inline comment that explains why the collected requests are currently filtered, on line 97) because it will require more changes and I would like to bring the netmonitor panel for the oop extensions at least on par with the non-oop extensions asap(mainly because currently  there is no other way for an addon developer to inspect the network requests originated from extension pages on windows).
Flags: needinfo?(poirot.alex)
(In reply to Luca Greco [:rpl] from comment #18)
> I also had to add a check if Services.appinfo was actually
> defined because it was failing on a worker test (because it seems that
> Services.appinfo was undefined there).

That's because modules in workers don't have access to any xpcom nor any privileged API expect a very limited API related to debugger API. There is a magic global "isWorker" injected in everymodule, which is true when the module runs to debug a worker.
You can check that instead of appinfo existence if you find it better. TBH I don't see much different between both checks.

> I'm not sure about the customNetworkRequest, I tried to find what it is its
> role by seaching in the rest of the devtools internals but I've not yet
> found where and how it is used, and so I have not changed it yet.

Yes I confirm, all console actors fields aren't used anymore.
Flags: needinfo?(poirot.alex)
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

https://reviewboard.mozilla.org/r/221472/#review231992

Thanks Luca, looks good to me.
I just hope we will followup someday sooner than later to simplify browser toolbox tests.
I was wondering if it would be hard to implement something like ContentTask but for the browser toolbox...
I opened bug 1444064 to finally start discussing improving this.

::: browser/components/extensions/test/browser/browser_ext_addon_debugging_netmonitor.js:129
(Diff revision 3)
> +  await extension.awaitFinish("netmonitor_request_logged");
> +
> +  let onToolboxClose = browserToolboxProcess.once("close");
> +  browserToolboxProcess._dbgProcessPromise.catch((err) => {
> +    dump(`BrowserToolboxProcess Error raised on close: ${err}\n`);
> +  });

I would rather suggest tweaking browser toolbox code rather than doing this only in this test.

I imagine you did that because it was throwing within these lines:
https://searchfox.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#303-322
And the rejection handler here wasn't called?
https://searchfox.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#323-325

Or is it just because console.log doesn't appear anywhere?
So either replace console.log by dump and/or convert it to use `catch()`.

::: devtools/server/actors/webconsole.js:90
(Diff revision 3)
>                               "last-pb-context-exited");
>    }
>  
>    this.traits = {
> -    customNetworkRequest: !this._parentIsContentActor,
> +    customNetworkRequest: !this._parentIsContentActor &&
> +      !this._parentIsWebExtensionChildActor,

It looks like only evaluateJSAsync and selectedObjectActor are still used.
So it may be easier to just get rid of it and parentIsContentActor as it will be the only one usage after this patch.
Attachment #8952217 - Flags: review+
(In reply to Alexandre Poirot [:ochameau] from comment #20)
> Yes I confirm, all console actors fields aren't used anymore.

This is poorly sentenced. All fields are not necessarely used, some are. But not customNetworkRequest.
Comment on attachment 8952217 [details]
Bug 1435959 - Fix missing network requests in netmonitor panel for oop extensions.

https://reviewboard.mozilla.org/r/221472/#review231992

> I would rather suggest tweaking browser toolbox code rather than doing this only in this test.
> 
> I imagine you did that because it was throwing within these lines:
> https://searchfox.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#303-322
> And the rejection handler here wasn't called?
> https://searchfox.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#323-325
> 
> Or is it just because console.log doesn't appear anywhere?
> So either replace console.log by dump and/or convert it to use `catch()`.

ouch, that's actually a leftover, I can't actually recall when I added it, the console message emitted by the ToolboxProcess.jsm is actually visible I'm pretty sure that I added it temporarily while I was putting together the "setupToolboxProcessTest" test helper and I wanted to see the error without searching it in the amount of logs that the test was already producing.

I've removed it, thanks for spotting it!

> It looks like only evaluateJSAsync and selectedObjectActor are still used.
> So it may be easier to just get rid of it and parentIsContentActor as it will be the only one usage after this patch.

great, that was what also my guess, I've removed it (and the `_parentIs*` methods) in the updated patch.
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/e561c88fc549
Fix missing network requests in netmonitor panel for oop extensions. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/e561c88fc549
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Attached image net monitor.gif
Reproduced issue on Firefox 60.0a1 (20180206100151) on Windows 10 64Bit.
Retested and verified in Firefox 60.0a1 (20180312100129) on Windows 10 64Bit, Mac OS 10.13.2.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: