1142292 - [e10s] cookie list fails

Status: VERIFIED FIXED

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

Description

[Geoffroy Planquart]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150310004228

Steps to reproduce:

./mach build
./mach run
press shift + F2 to open gcli
type "cookie list" then enter


Actual results:

On UI: TypeError: this.window is null

In the program output:

console.error: 
  Message: TypeError: this.window is null
  Stack:
    CommandUtils.createEnvironment/<.document@resource:///modules/devtools/DeveloperToolbar.jsm:224:9
exports.items<.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/cookie.js:48:11
Requisition.prototype.exec/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2098:16
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:239:18
Task_spawn@resource://gre/modules/Task.jsm:164:12
exports.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/host.js:72:10
Requisition.prototype.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2097:14
Inputter.prototype._handleReturn@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/mozui/inputter.js:563:12
Inputter.prototype.handleKeyUp@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/mozui/inputter.js:462:1
Inputter.prototype.onKeyUp@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/mozui/inputter.js:434:3


Expected results:

It should have listed cookies or told me that there aren't any cookies.

Comment 1

[:Gijs (he/him)]

Seems e10s-related.

Comment 2

[:Gijs (he/him)]

I can repro on Windows, too -> platform/os = all/all.

Comment 3

[Patrick Brosset <:pbro>]

GCLI isn't compatible with e10s just yet, and I'm almost certain that's what's causing this issue.
We're getting there. jwalker is working on this in bug 1128988.

Comment 4

[Geoffroy Planquart]

Thanks for the feedback.

Quick & dirty fix which surprisingly works:

diff --git a/browser/devtools/shared/DeveloperToolbar.jsm b/browser/devtools/shared/DeveloperToolbar.jsm
index 9c2375e..ded0d95 100644
--- a/browser/devtools/shared/DeveloperToolbar.jsm
+++ b/browser/devtools/shared/DeveloperToolbar.jsm
@@ -217,7 +217,7 @@ let CommandUtils = {
       },
 
       get window() {
-        return this.chromeWindow.gBrowser.selectedBrowser.contentWindow;
+        return this.chromeWindow.gBrowser.selectedBrowser._contentWindow;
       },
 
       get document() {

Comment 5

[janekptacijarabaci]

Build:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0 ID:20150516030207

Current results:
"cookieMgr is not defined"

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I get the following error in Nightly 41:

Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]

resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/cookie.js:13:10
XPCU_defineLazyGetter/<.get@resource://gre/modules/XPCOMUtils.jsm:196:21
exports.items<.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/cookie.js:70:11
Requisition.prototype.exec/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2073:16
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:239:18Task_spawn@resource://gre/modules/Task.jsm:164:12
exports.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/host.js:72:10
Requisition.prototype.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2072:14
Requisition.prototype.updateExec/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2112:12
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23

which points to the same trouble with the cookie service that the storage inspector is having.  We'll probably solve the issue there, so let's depend on it.

Comment 7

[tobymathews]

I have this in version 40.0a2 - first I get:

Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)

and then if I try 'cookie list' again I get:

cookieMgr is not defined

Comment 10

[Patrick Brosset <:pbro>]

My understanding of the problem is that nsICookieManager2 isn't available in child processes.
However the 'cookie list' command is set to run on the server (same with the 'cookie remove' and 'cookie set' commands).
Running on the server makes sense because we need the command to run on the debuggee, which could be a remote device. But that being said, once the command starts executing on the server, it should then reach out to its parent process to get the list of cookies from there.

You can confirm this by changing 'runAt: "server"' to 'runAt: "client"' in the cookie.js gcli commands file and executing 'cookie list' on firefox desktop with e10s.

This is very similar to what the storageActor does.
The command would need to setup a script in the parent process, using something like:

    this.conn.setupInParent({
      module: "path/to/the/cookie-command-script-in-parent",
      setupParent: "setupMethodName"
    });

(by the way, path/to/the/cookie-command-script-in-parent could be the command file itself to avoid having to create another one).

And then, setupMethodName should start listening to child process messages like list, remove, set and respond accordingly.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #10)
>     this.conn.setupInParent({
>       module: "path/to/the/cookie-command-script-in-parent",
>       setupParent: "setupMethodName"
>     });
The problem here is that there's potentially no active DebuggerServer connections, the toolbox might not have been opened before. So I don't think setupInParent can be used.

Joe, can gcli help server commands run code in the server's parent process? Do you think having a 'runAt: "server-parent"' mode would be possible?

Comment 12

[Joe Walker [:jwalker]]

runAt: "server-parent" is a really good idea.

A temporary workaround might be to say runAt:client and then set the command to hidden:isFxDesktop or something nasty like that. That way toolboxes that are talking to a non desktop process won't have the command. Ug, though.

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #12)
> runAt: "server-parent" is a really good idea.
Filed bug 1221488 for it.

Comment 14

[Patrick Brosset <:pbro>]

Attachment: Bug_1142292_-_Run_cookie_gcli_commands_on_the_clie.diff

Would you say this is an acceptable work-around until bug 1221488 lands?
The developer toolbar is only accessible on desktop firefox anyway, and the cookie commands aren't called from anywhere else today. So we can safely assume that they always run on a desktop firefox when debugging a local tab.

Comment 15

[Joe Walker [:jwalker]]

Comment on attachment 8683651 [details] [diff] [review]
Bug_1142292_-_Run_cookie_gcli_commands_on_the_clie.diff

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

Is there an easy way to say add

    get hidden() {
      return !isRemoteFirefoxDesktop;
    }

To the command definition?

i.e. a variant of https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/calllog.js#103

    get hidden() {
      return gcli.hiddenByChromePref();
    },

So we could hide the command in places where it wouldn't work. I'm guessing that's not easy.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #15)
> Comment on attachment 8683651 [details] [diff] [review]
> Bug_1142292_-_Run_cookie_gcli_commands_on_the_clie.diff
> 
> Review of attachment 8683651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there an easy way to say add
> 
>     get hidden() {
>       return !isRemoteFirefoxDesktop;
>     }
> 
> To the command definition?
> 
> i.e. a variant of
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/
> calllog.js#103
> 
>     get hidden() {
>       return gcli.hiddenByChromePref();
>     },
> 
> So we could hide the command in places where it wouldn't work. I'm guessing
> that's not easy.

I've been trying to find a way to do this, maybe the command could check if there's an active toolbox with a remote connection or something. But the thing is, the developertoolbar doesn't work with remote connections. When you open it, it always targets the local firefox tab it's attached to. So there's no way for it to execute commands over a remote connection.

So the only risk here is if someone tries to execute the cookie commands from toolbox buttons (which can run commands on remote connections). Someone trying to do this would have to modify the cookie.js file and would find out about the restriction.
But in order to further mitigate this, I guess I could add in all `exec` functions a check for:
context.environment.target.isRemote
and if `true`, then throw a developer-friendly error.

Comment 17

[Patrick Brosset <:pbro>]

Attachment: Bug_1142292_-_Run_cookie_gcli_commands_on_the_clie.diff

I know that you already R+'d this, but this is just to make sure my reasoning in comment 16 is sane.

Comment 18

[Patrick Brosset <:pbro>]

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8683651&action=interdiff&newid=8684138&headers=1

Comment 19

[Joe Walker [:jwalker]]

Comment on attachment 8684138 [details] [diff] [review]
Bug_1142292_-_Run_cookie_gcli_commands_on_the_clie.diff

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

Thanks.

Comment 20

[Patrick Brosset <:pbro>]

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

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/beb71b4e9def

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/beb71b4e9def

Comment 23

[Kazi Nuzhat Tasnem]

I have reproduced this bug with Firefox nightly 44.0a1(build id:20150310004228)on 
windows 7(64 bit)

I have verified this bug as fixed with Firefox beta 45.0b6(build id:20160215141016)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.

[testday-20160219]

Comment 24

[:Gijs (he/him)]

(In reply to nuz.tasneem from comment #23)
> I have reproduced this bug with Firefox nightly 44.0a1(build
> id:20150310004228)on 
> windows 7(64 bit)
> 
> I have verified this bug as fixed with Firefox beta 45.0b6(build
> id:20160215141016)
> User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
> Firefox/45.
> 
> [testday-20160219]

Thanks!