Closed Bug 1042859 Opened 7 years ago Closed 6 years ago

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

Categories

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

31 Branch
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: jacobsenscott, Assigned: geoffroy)

Details

Attachments

(3 files, 5 obsolete files)

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.
Component: Untriaged → Developer Tools: Graphic Commandline and Toolbar
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
Same here!
(In reply to enrico from comment #2)
> Same here!

Firefox 32 on Arch Linux
Still  the same issue with FDE 37.0a2 on Mac. 

"Keine Cookies für Host localhost:8888 gefunden"
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 ?
(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)
> 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
Depends on: 1142292
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.
Thanks pbrosset, I was about to specify it. Can you remove the dependance on bug 1142292 then?
No longer depends on: 1142292
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.
I found that cookies are stored without the port so I'm simply stripping the port number if any from the host name.
Attachment #8577197 - Attachment is obsolete: true
Attachment #8577201 - Flags: review?(pbrosset)
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.
Attachment #8577201 - Flags: review?(pbrosset)
Assignee: nobody → geoffroy
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Okay, I'll take a look.

I'm also working on bug 925943 in the same file.
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.
Attachment #8577201 - Attachment is obsolete: true
Attachment #8577208 - Flags: review?(pbrosset)
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.
Attachment #8577208 - Flags: review?(pbrosset) → review+
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.
Added a test case which fails without the other patch.
Attachment #8577256 - Flags: review?(pbrosset)
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/ */
Attachment #8577256 - Flags: review?(pbrosset)
Done!

Thanks for the tips ;)
Attachment #8577256 - Attachment is obsolete: true
Attachment #8577270 - Flags: review?(pbrosset)
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.
Attachment #8577270 - Flags: review?(pbrosset) → review+
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
Attachment #8577208 - Attachment is obsolete: true
Attachment #8577277 - Flags: review+
Attachment #8577278 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/051cb67813e3
https://hg.mozilla.org/mozilla-central/rev/cb6a2374cfa1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
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.
(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.
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.