1042859 - Dev toolbar does not display cookies for a web app running on localhost:3000

Status: RESOLVED FIXED

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

Description

[scott]

Attachment: Log of request and response headers

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Run a web app on localhost:3000 that sets cookies.

Open the dev toolbar and enter "cookie list"


Actual results:

Displays "No cookies found for host localhost:3000"


Expected results:

Cookies should be listed.

Firebug does show the cookies. Chrome dev tools also show the cookies.

Comment 1

[william.w.cline]

I'm experiencing this, too. Firefox 31.0 on Mac OS 10.9.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0

Comment 2

[enrico]

Same here!

Comment 3

[enrico]

(In reply to enrico from comment #2)
> Same here!

Firefox 32 on Arch Linux

Comment 4

[damahevi]

Still  the same issue with FDE 37.0a2 on Mac. 

"Keine Cookies für Host localhost:8888 gefunden"

Comment 5

[Geoffroy Planquart]

I'm having the same problem on Firefox 35 / FDE 38 / nightly 39 on archlinux.

I've got an idea for a patch, can I take this issue ?

Comment 6

[:Gijs (he/him)]

(In reply to Geoffroy Planquart from comment #5)
> I'm having the same problem on Firefox 35 / FDE 38 / nightly 39 on archlinux.
> 
> I've got an idea for a patch, can I take this issue ?

Definitely! Feel free to do this if you (think you) know how to fix it!

If you need help building Firefox, try #introduction on IRC, or #devtools specifically about devtools issues like this. ( https://wiki.mozilla.org/IRC for background on IRC)

Comment 7

[Geoffroy Planquart]

> Definitely! Feel free to do this if you (think you) know how to fix it!

Should be pretty straightforward... Instead of looking for cookies for localhost:3000 I'll fall back to localhost alone (iff localhost).

> If you need help building Firefox, try #introduction on IRC, or #devtools
I just created a bootstrapper for archlinux (bug 942475) and I can build now..

However, I can't get `cookie list' to work on my build, with or without my patch:

    TypeError: this.window is null
    CommandUtils.createEnvironment/<.document@resource:///modules/devtools/DeveloperToolbar.jsm:225:9

I'll investigate on this later, but AFAIK it is not related to bug 1111276, the problem is the same with or without this diff: https://hg.mozilla.org/mozilla-central/diff/1439cb361d99/browser/devtools/shared/DeveloperToolbar.jsm

Comment 8

[Patrick Brosset <:pbro>]

As said in bug 1142292, gcli doesn't work with e10s yet, so I suggest you open non-e10s windows to continue your work on this bug (under the File menu bar item, choose New non-e10s window). There might be a pref you can set too, but I forgot which one.

Comment 9

[Geoffroy Planquart]

Thanks pbrosset, I was about to specify it. Can you remove the dependance on bug 1142292 then?

Comment 10

[Geoffroy Planquart]

Attachment: Bug-1042859-Fallback-to-localhost-when-no-cookies-for-port.patch

Really simple patch which fallbacks to localhost when localhost:port does not have any cookie.

I'd like to test on another website (not localhost) using non-standard port if the behavior is the same, in which case I'll update the patch.

I think Firefox may store the cookies by host name without any distinction of port number.

Comment 11

[Geoffroy Planquart]

Attachment: Bug-1042859-Ignoring-port-when-fetching-cookies.patch

I found that cookies are stored without the port so I'm simply stripping the port number if any from the host name.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8577201 [details] [diff] [review]
Bug-1042859-Ignoring-port-when-fetching-cookies.patch

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

This looks like a nice and simple fix, thanks.
However, I think the 'set' and 'remove' commands also need fixing, I don't seem to make them work on a localhost:port webpage either.

Comment 13

[Geoffroy Planquart]

Okay, I'll take a look.

I'm also working on bug 925943 in the same file.

Comment 14

[Geoffroy Planquart]

Attachment: Bug-1042859-Ignoring-port-when-fetching-cookies.patch

New patch sanitizing the host for every methods in cookies.js

I created the function sanitizeHost for this purpose, so the same check is done in every method.

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8577208 [details] [diff] [review]
Bug-1042859-Ignoring-port-when-fetching-cookies.patch

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

Looks good to me. R+ for the code changes. I'm wondering whether it's possible to create a test for this.

Comment 16

[Patrick Brosset <:pbro>]

It looks like testing this should be possible. I'm not really sure how this works, but it seems the mochitest test harness starts a server when tests run that is accessible via a range of different URLs.
In a lot of tests, we load the test page with http://example.com/browser/browser/devtools/... but some tests also use http://mochi.test:8888/browser/browser/devtools/...
which should be enough to make the command fail without your patch, right?
If yes, please create a new test file next in the same directory as this one:
/browser/devtools/commandline/test/browser_cmd_cookie.js
It could be named something like browser_cmd_cookie_host.js
It should look a lot like browser_cmd_cookie.js except the TEST_URI should have the port, and you wouldn't need to have so many test cases in helpers.audit.
Just a few test cases that try to add cookies, list them and remove them.

Comment 17

[Geoffroy Planquart]

Attachment: Bug-1042859-Added-test-case-for-cookies-on-host-port.patch

Added a test case which fails without the other patch.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8577256 [details] [diff] [review]
Bug-1042859-Added-test-case-for-cookies-on-host-port.patch

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

Just a couple of minor changes needed and it's good to go.
Can you make the change, upload the new version (marking the old version on bugzilla as obsolete) and ask for review again?
Once done and R+'d, I'll push to the TRY server to make sure all tests still pass fine.

::: browser/devtools/commandline/test/browser.ini
@@ +34,5 @@
>  support-files =
>   browser_cmd_cookie.html
> +[browser_cmd_cookie_host.js]
> +support-files =
> + browser_cmd_cookie.html

You don't actually need to re-define the support-files section here because the browser_cmd_cookie.html file has already been flagged as a support-file before.
It's bit misleading that support-files are defined separately and per test in this browser.ini file. If you look at this one for instance: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser.ini all support-files are part of just one section at the top, so it's easier to understand how the whole thing is supposed to be done.
I'm not saying you should change this browser.ini file though, just remove these 2 last lines.

::: browser/devtools/commandline/test/browser_cmd_cookie_host.js
@@ +1,1 @@
> +// Tests that the cookie command works for host with a port specified

This new file needs the same copyright header than the other test files:

/* Any copyright is dedicated to the Public Domain.
* http://creativecommons.org/publicdomain/zero/1.0/ */

Comment 19

[Geoffroy Planquart]

Attachment: Bug-1042859-Added-test-case-for-cookies-on-host-port.patch

Done!

Thanks for the tips ;)

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8577270 [details] [diff] [review]
Bug-1042859-Added-test-case-for-cookies-on-host-port.patch

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

Thanks. That looks good.
Here's a try push with those 2 patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e81f6a4ca05
When everything's completed and green, we can land the patches.
Can you keep an eye on this? I usually takes a few hours to complete running all tests on all machines.

Comment 21

[Patrick Brosset <:pbro>]

Oh and one last thing. Now that the patches are R+, can you re-format the commit messages please?
Bug <bugnumber> - <short description of changes done>; r=<username of reviewer>
So in your case:
Bug 1042859 - Ignore host port when fetching cookies with the gcli cookie command; r=pbrosset

Comment 22

[Geoffroy Planquart]

Attachment: Bug-1042859-Ignoring-port-when-fetching-cookies.patch

Comment 23

[Geoffroy Planquart]

Attachment: Bug-1042859-Added-test-case-for-cookies-on-host-port.patch

Comment 24

[Patrick Brosset <:pbro>]

New pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9c2601661f8

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/051cb67813e3
https://hg.mozilla.org/integration/fx-team/rev/cb6a2374cfa1

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/051cb67813e3
https://hg.mozilla.org/mozilla-central/rev/cb6a2374cfa1

Comment 27

[Lindsay Pallickal]

I am on Firefox 41.0.1 on Xubuntu, and setting cookies for my server at http://localhost:8000/index.html or http://myAlternateHostnameFromEtcHosts:8000/index.html, and also file://directory/index.html, does work via JavaScript, but the cookies do not show up in storage inspector under developer tools. The cookie persists and can be read back for display after a page reload.

I stumbled upon this page while searching for a solution, and it seems to me somehow this bug still persists. I am using angular.js $cookies.set and $cookies.get functions, although it should not matter versus using raw JavaScript.

Comment 28

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

(In reply to pallickal from comment #27)
> I am on Firefox 41.0.1 on Xubuntu, and setting cookies for my server at
> http://localhost:8000/index.html or
> http://myAlternateHostnameFromEtcHosts:8000/index.html, and also
> file://directory/index.html, does work via JavaScript, but the cookies do
> not show up in storage inspector under developer tools. The cookie persists
> and can be read back for display after a page reload.
> 
> I stumbled upon this page while searching for a solution, and it seems to me
> somehow this bug still persists. I am using angular.js $cookies.set and
> $cookies.get functions, although it should not matter versus using raw
> JavaScript.

This bug is not about the storage inspector, so it is not the same as what you are describing.  I think it would be best to file a new bug about your issue, so it can be properly tracked and investigated.