The storage inspector of devtools depends on this 'getCookiesFromHost' to acquire cookies for a specific host. But now, this API uses default user context, which causes the storage inspector cannot acquire correct cookies when user context id involves. We should make this API origin attributes aware.

The main risk to keep in mind here is addon compatability, since this is a very popular API: https://mxr.mozilla.org/addons/search?string=getcookiesfromhost

(In reply to Josh Matthews [:jdm] from comment #1) > The main risk to keep in mind here is addon compatability, since this is a > very popular API: > https://mxr.mozilla.org/addons/search?string=getcookiesfromhost Josh, thanks for reminding us! Tim and I consulted Andrea (baku) about how to ensure we don't break Addons while changing this API. We will mainly reply on tests in extensions/cookie/test/unit/ and somewhere else.

I think Tim is also referring to bug 1259169 (nsICookieManager::remove() should be back-compatible for 1 or 2 releases).

It looks like that the storage inspector also uses nsICookieManager2.add to edit cookies [1]. I think I should make the add() origin attributes aware as well. But should we only modify these two only? Or should we make all APIs of nsICookieManager2 origin attributes aware? [1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/storage.js#887

(In reply to Tim Huang[:timhuang] from comment #4) > It looks like that the storage inspector also uses nsICookieManager2.add to > edit cookies [1]. I think I should make the add() origin attributes aware > as well. But should we only modify these two only? Or should we make all > APIs of nsICookieManager2 origin attributes aware? > > [1] > https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/ > storage.js#887 What is the difference between nsICoookieManager and nsICookieManager2? Looking at the files, it looks like one is just an extension of another. Is that right? We have added a second remove() method to nsICookieManager to make it compatible with origin attributes. We have also kept the original remove() method so that addons would not break. We could certainly make add() and getCookiesForHost() compatible with origin attributes, by adding an optional parameter or creating a second method that takes OriginAttributes. Same for cookieExists and countCookiesFromHost, but they are lower priority. How does importCookies work? Is it capable of importing cookies with originAttributes? If not and if we allowing importing cookies from one Firefox profile to another, we should add originAttribute support to importCookies. Are getCookiesForApp and removeCookiesForApp deprecated? If not, we may be able to remove getCookiesForApp and removeCookiesForApp entirely and either: 1) use getCookiesFromHost(pass in nothing for host, pass in originAttributes with the appid). Same with remove 2) replace with getCookiesForOriginAttributes() and removeCookiesFromOriginAttributes() that will find all cookies with specific originattributes and remove them. This isn't great though for a case where we have both an appid and a usercontextid set. Maybe we want to remove all cookies with appid X regardless of what the usercontext id is. We'd have to call getCookiesForOriginAttributes multiple times with rotating usercontextids and one single appid.

(In reply to Tanvi Vyas [:tanvi] from comment #5) > (In reply to Tim Huang[:timhuang] from comment #4) > > It looks like that the storage inspector also uses nsICookieManager2.add to > > edit cookies [1]. I think I should make the add() origin attributes aware > > as well. But should we only modify these two only? Or should we make all > > APIs of nsICookieManager2 origin attributes aware? > > > > [1] > > https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/ > > storage.js#887 > > What is the difference between nsICoookieManager and nsICookieManager2? > Looking at the files, it looks like one is just an extension of another. Is > that right? Yes, the nsICookieManager2 extend capabilities of nsICookieManager because nsICookieManager is in a frozen state. > We have added a second remove() method to nsICookieManager to make it > compatible with origin attributes. We have also kept the original remove() > method so that addons would not break. For any change here, I will follow the way that remove() did to prevent break any addon. > We could certainly make add() and getCookiesForHost() compatible with origin > attributes, by adding an optional parameter or creating a second method that > takes OriginAttributes. > > Same for cookieExists and countCookiesFromHost, but they are lower priority. So, I think I should focus on add() and getCookiesForHost() in the scope of this bug. > How does importCookies work? Is it capable of importing cookies with > originAttributes? If not and if we allowing importing cookies from one > Firefox profile to another, we should add originAttribute support to > importCookies. No, it does not import cookies with originAttributes. In fact, it alwasy utilzes default originAttributes when importing cookies. But I don't know that do we allow importing cookies from other profile, maybe baku can answer this question. And actually, little function uses importCookies in the gecko codebase, I think this should be low priority as well. > Are getCookiesForApp and removeCookiesForApp deprecated? If not, we may be > able to remove getCookiesForApp and removeCookiesForApp entirely and either: > 1) use getCookiesFromHost(pass in nothing for host, pass in originAttributes > with the appid). Same with remove > 2) replace with getCookiesForOriginAttributes() and > removeCookiesFromOriginAttributes() that will find all cookies with specific > originattributes and remove them. This isn't great though for a case where > we have both an appid and a usercontextid set. Maybe we want to remove all > cookies with appid X regardless of what the usercontext id is. We'd have to > call getCookiesForOriginAttributes multiple times with rotating > usercontextids and one single appid. The Bug 1214071 is going to take care of this part.

> No, it does not import cookies with originAttributes. In fact, it alwasy > utilzes default originAttributes when importing cookies. But I don't know > that do we allow importing cookies from other profile, maybe baku can answer > this question. To me it makes sense that the importing of cookies uses the Necko default OriginAttributes. First of all, the file doesn't contain any information about the originAttributes; then, we don't want to have the same cookies everywhere because that breaks the reason why we are implementing this container feature. > And actually, little function uses importCookies in the gecko codebase, I > think this should be low priority as well. Indeed.

(In reply to Andrea Marchesini (:baku) from comment #7) > > No, it does not import cookies with originAttributes. In fact, it alwasy > > utilzes default originAttributes when importing cookies. But I don't know > > that do we allow importing cookies from other profile, maybe baku can answer > > this question. > > To me it makes sense that the importing of cookies uses the Necko default > OriginAttributes. > First of all, the file doesn't contain any information about the > originAttributes; then, we don't want to have the same cookies everywhere > because that breaks the reason why we are implementing this container > feature. > Okay, as long as the cookies we are importing don't specify a usercontextid, we can set them to usercontextid 0. In what cases is import from cookies called? Importing from the OS, other browsers, an addon?

Kamil, can you test the behavior of importing cookies across Firefox profiles?

Update nsICookieManager2.idl.

Because ehsan is busy right now, josh could you help me on reviewing my patches?

Fix some issues.

Comment on attachment 8750168 [details] [diff] [review] Part 1 - Make the API getCookiesFromHost of the nsICookieManager2 OriginAttributes-aware. Review of attachment 8750168 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cookie/nsCookieService.cpp @@ +4376,5 @@ > nsAutoCString baseDomain; > rv = GetBaseDomainFromHost(host, baseDomain); > NS_ENSURE_SUCCESS(rv, rv); > > + NeckoOriginAttributes attrs; Since this code is repeated multiple times, can we extract it into a helper function like nsresult InitializeOriginAttributes(NeckoOriginAttributes* aAttrs, JS::HandleValue aOriginAttributes, JSContext* aCx, uint8_t aArgc, const char* aAPI, const char* aInterfaceSuffix)?

Comment on attachment 8750592 [details] [diff] [review] part 2 - Update all existing functions of add() and getCookiesFromHost() to make them origin attributes aware. Review of attachment 8750592 [details] [diff] [review]: ----------------------------------------------------------------- A devtools person should look at the devtools changes.

Comment on attachment 8750647 [details] [diff] [review] part 3 - Add test cases for add() and getCookiesFromHost() of the nsICookieManager2. Review of attachment 8750647 [details] [diff] [review]: ----------------------------------------------------------------- I haven't seen the equal/ok assertion APIs in xpcshell tests before (only do_check_eq/do_check_true), but I'm assuming that it works as expected! ::: netwerk/cookie/test/unit/test_bug1267910.js @@ +46,5 @@ > + > +function checkCookie(cookie, cookieObj) { > + for (let prop of Object.keys(cookieObj)) { > + if (prop === "originAttributes") { > + ok(ChromeUtils.isOriginAttributesEqual(cookie[prop],cookieObj[prop]), nit: space after , @@ +57,5 @@ > + > +function countCookies(enumerator) { > + let cnt = 0; > + > + while(enumerator.hasMoreElements()) { nit: space after while @@ +105,5 @@ > + let foundCookie = enumerator.getNext().QueryInterface(Ci.nsICookie2); > + > + checkCookie(foundCookie, COOKIE); > + > + ok(!enumerator.hasMoreElements(), "We should get only one cooike"); cookie @@ +165,5 @@ > + foundCookie = enumerator.getNext().QueryInterface(Ci.nsICookie2); > + checkCookie(foundCookie, COOKIE_OA_1); > + > + // We should only get one cookie. > + ok(!enumerator.hasMoreElements(), "We should get only one cooike"); cookie

Comment on attachment 8750592 [details] [diff] [review] part 2 - Update all existing functions of add() and getCookiesFromHost() to make them origin attributes aware. Mike could you take a look of the change of the storage inspector regarding cookies?

Changing the code according to the comment 16.

Fix issues and typos that comment 18 describes.

Update commit message.

Try test looks good.

Part 3 needs rebasing.

Rebase from mozilla-central.

Rebase from mozilla-central.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc202e0ac0dd https://hg.mozilla.org/integration/mozilla-inbound/rev/f8afc5cf9e0e https://hg.mozilla.org/integration/mozilla-inbound/rev/5962f8e6d030

https://hg.mozilla.org/mozilla-central/rev/cc202e0ac0dd https://hg.mozilla.org/mozilla-central/rev/f8afc5cf9e0e https://hg.mozilla.org/mozilla-central/rev/5962f8e6d030

f8afc5cf9e0e has broken session restore for me. All the windows and tabs open, but they are all blank (the location bar is empty for all of them). The child process also does not start.

Also seeing reports on Mozillazine that this broke sessionrestore: http://forums.mozillazine.org/viewtopic.php?p=14608385#p14608385 Backed out in https://hg.mozilla.org/mozilla-central/rev/f1f2644d3444

(In reply to Wes Kocher (:KWierso) from comment #31) > Backed out in https://hg.mozilla.org/mozilla-central/rev/f1f2644d3444 I can confirm that session restore is now working again.

The reason why session store does not work is that the cookies store in the sessionrestore data doesn't contain originAttributes before this bug. Therefore, when restoring such data, the cookie.originAttributes would be undefined which will cause the nsICookieManager2.add() throws a exception and makes sessionrestore broken. For addressing this problem, I will assign the originAttributes as default if there is no originAttributes in the cookies of the sessionrestore data.

We also really need a session restore regression test that would have caught this. We need more Nightly users, and breaking session restore is a good way to get less. ;)

Comment on attachment 8752133 [details] [diff] [review] Part 1 - Make the API getCookiesFromHost of the nsICookieManager2 OriginAttributes-aware. Review of attachment 8752133 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/locales/en-US/necko.properties @@ +44,5 @@ > # %1$S is the deprected API; %2$S is the API function that should be used. > APIDeprecationWarning=Warning: ‘%1$S’ deprecated, please use ‘%2$S’ > > +# LOCALIZATION NOTE (nsICookieManagerDeprecated): don't localize originAttributes. > +nsICookieManagerAPIDeprecated=“%1$S” is changed. Update your code and pass the correct originAttributes. Read more on MDN: https://developer.mozilla.org/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager%2$S Before re-landing, please add a proper localization note explaining what are the values for %1$S and %2$S. For %2$S in particular, this is not clear.

Add localization notes.

Solving the sessionstore crash problem based on comment 33.

Add a test case for the regression test that will prevent this problem happens again.

Comment on attachment 8755195 [details] [diff] [review] Part 4 - Add a test case for the session store regression test. Review of attachment 8755195 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting this review to someone who knows how to read and write browser UI tests.

(In reply to Tim Huang[:timhuang] from comment #37) > Created attachment 8755194 [details] [diff] [review] > part 2 - Update all existing functions of add() and getCookiesFromHost() to > make them origin attributes aware. > > Solving the sessionstore crash problem based on comment 33. This is a large patch; could you show me a diff between the previous one and this one so I can actually review the changes since the last time?

This is the inter diff of the part2. Josh, thanks for reviewing my code.

Mike, thanks for your advices, template literals are really helpful. This patch address your comments.

(In reply to Tim Huang[:timhuang] from comment #47) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcad4b8f6bd7 I had to cancel this one, because it was doing too much... Did you know you can customize your try push using http://trychooser.pub.build.mozilla.org/ to save time and resources? You only need to check 'mochitest-bc' to be run there, all the other tests are not necessary. Please ping me on IRC if you'd like to learn more. Thanks!

Comment on attachment 8756377 [details] [diff] [review] Part 4 - Add a test case for the session store regression test. Review of attachment 8756377 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/test/browser_restore_cookies_noOriginAttributes.js @@ +6,5 @@ > + > +const TEST_HOST = "www.example.com"; > +const COOKIE = { name: "test1", > + value: "yes1", > + path: "/browser/browser/components/sessionstore/test/"}; nit: can you format this as: ```js { name: "test1", value: "yes1", path: "/browser/browser/components/sessionstore/test/" }; ``` ? @@ +8,5 @@ > +const COOKIE = { name: "test1", > + value: "yes1", > + path: "/browser/browser/components/sessionstore/test/"}; > +const SESSION_DATA = `{ > + "version": ["sessionrestore", 1], nit: please start the indentation with two spaces, not this much. Same for the literal below.

Comment on attachment 8755195 [details] [diff] [review] Part 4 - Add a test case for the session store regression test. Review of attachment 8755195 [details] [diff] [review]: ----------------------------------------------------------------- There are quite a few issues here. Please have a look below. For the next iteration, I think it'd be a good idea to get review from :mikedeboer, who's been looking at session store generally anyway. ::: browser/components/sessionstore/test/browser.ini @@ +53,5 @@ > restore_redirect_http.html^headers^ > restore_redirect_js.html > restore_redirect_target.html > browser_1234021_page.html > + browser_1267910_sessionstore.js Please add support files under the test they belong to rather than under [default]. @@ +227,5 @@ > [browser_parentProcessRestoreHash.js] > run-if = e10s > [browser_sessionStoreContainer.js] > [browser_1234021.js] > +[browser_1267910.js] Please name the test after what, specifically, you're testing, e.g. browser_restoreSession_noOriginAttributes.js or something. ::: browser/components/sessionstore/test/browser_1267910.js @@ +13,5 @@ > +const COOKIE = { name: "test1", > + value: "yes1", > + path: "/browser/browser/components/sessionstore/test/"} > + > +function promiseRead(path) { You don't use this, so please get rid of it. @@ +19,5 @@ > +} > + > +add_task(function* init() { > + // Wait until initialization is complete > + yield SessionStore.promiseInitialized; Just put this in the main task. @@ +26,5 @@ > + > +add_task(function* run_test() { > + // First, overwirte the sessionstore backup file with the one which doesn't > + // contain originAttributes in the cookies session. > + yield OS.File.copy(getTestFilePath("browser_1267910_sessionstore.js"), Paths.clean); So... generally, this seems wrong to me, for a unit test. You could just have the relevant window state in a JSON object in this file, and call setWindowState directly with that state, and then check cookies got restored. This would be a lot simpler and wouldn't require overwriting any backup files, and potentially racing if something else then overwrites the backup file with the 'correct' state before you manage to restore the state. @@ +28,5 @@ > + // First, overwirte the sessionstore backup file with the one which doesn't > + // contain originAttributes in the cookies session. > + yield OS.File.copy(getTestFilePath("browser_1267910_sessionstore.js"), Paths.clean); > + > + let cs = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2); Services.cookies @@ +44,5 @@ > + let tab = win.gBrowser.addTab("about:blank"); > + let browser = tab.linkedBrowser; > + > + // Wait the tab to be loaded. > + yield BrowserTestUtils.browserLoaded(browser); Replace this and the previous few lines with: let tab = yield BrowserTestUtils.openNewForegroundTab(win.gBrowser, "about:blank"); but really, why are we creating a tab anyway? It doesn't seem like we ever do anything with it - we're just checking the cookie aspect of the restore of the window. @@ +47,5 @@ > + // Wait the tab to be loaded. > + yield BrowserTestUtils.browserLoaded(browser); > + > + // Restore window > + ss.setWindowState(win, result.source, true); I don't think we can depend on this being synchronous in e10s. That means that the restore might still be in progress when we close the window. That *should* work, but I'll be curious to see if it really does if you push to try, and it seems no trypush happened since you created this test (at least, it is not on the bug). @@ +63,5 @@ > + is(cookie.value, COOKIE.value, "cookie value successfully restored"); > + is(cookie.path, COOKIE.path, "cookie path successfully restored"); > + > + > + yield promiseRemoveTab(tab); Not sure why we should bother with this if we're just closing the window afterwards. Just close the window in one go? Or is this about the 'close multiple tabs' warning or something? If so, it'd be a good idea to make that clear in a comment. If not, just get rid of this line. :-) If we're creating the tab in order that we can close it here, you can just use win.gBrowser.selectedTab instead. ::: browser/components/sessionstore/test/browser_1267910_sessionstore.js @@ +1,1 @@ > +{"version":["sessionrestore",1],"windows":[{"tabs":[{"entries":[],"lastAccessed":1463893009797,"hidden":false,"attributes":{},"image":null},{"entries":[{"url":"http://www.example.com/browser/browser/components/sessionstore/test/browser_1267910_page.html","charset":"UTF-8","ID":0,"docshellID":2,"originalURI":"http://www.example.com/browser/browser/components/sessionstore/test/browser_1267910_page.html","docIdentifier":0,"persist":true}],"lastAccessed":1463893009321,"hidden":false,"attributes":{},"userContextId":0,"index":1,"image":"http://www.example.com/favicon.ico"}],"selected":1,"_closedTabs":[],"busy":false,"width":1280,"height":747,"screenX":4,"screenY":23,"sizemode":"normal","cookies":[{"host":"www.example.com","value":"yes1","path":"/browser/browser/components/sessionstore/test/","name":"test1"}]}],"selectedWindow":1,"_closedWindows":[],"session":{"lastUpdate":1463893009801,"startTime":1463893007134,"recentCrashes":0},"global":{}} \ No newline at end of file Because it doesn't matter for the test, please can we format this in a readable manner? JSON beautifiers should be able to help with this.

Carrying over r=mikedeboer from comment 49.

Thanks for your review, Gijs, and this patch addresses your comments. Flag :mikedeboer to review this patch.

Comment on attachment 8756353 [details] [diff] [review] Part 4 - Add a test case for the session store regression test. Review of attachment 8756353 [details] [diff] [review]: ----------------------------------------------------------------- Please attach an updated patch when ready, along with a push to try. Getting close! ::: browser/components/sessionstore/test/browser_restoreSession_cookies_noOriginAttributes.js @@ +4,5 @@ > + > +"use strict"; > + > +const TEST_HOST = "www.example.com"; > + nit: no additional newline necessary. @@ +8,5 @@ > + > +const COOKIE = { name: "test1", > + value: "yes1", > + path: "/browser/browser/components/sessionstore/test/"}; > + same here. @@ +9,5 @@ > +const COOKIE = { name: "test1", > + value: "yes1", > + path: "/browser/browser/components/sessionstore/test/"}; > + > +const SESSION_DATA = "{\ This would be a great opportunity to learn about template literals! https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals Please learn about it and apply them here. It'll save you a lot of backslashes, for sure. @@ +61,5 @@ > + \"global\": {}\ > + }"; > + > +add_task(function* run_test() { > + // Wait until initialization is complete nit: missing dot. @@ +64,5 @@ > +add_task(function* run_test() { > + // Wait until initialization is complete > + yield SessionStore.promiseInitialized; > + > + // Clear cookies nit: missing dot. @@ +70,5 @@ > + > + // Open a new window. > + let win = yield promiseNewWindowLoaded(); > + > + // Restore window nit: missing dot. @@ +75,5 @@ > + ss.setWindowState(win, SESSION_DATA, true); > + > + let enumerator = Services.cookies.getCookiesFromHost(TEST_HOST, {}); > + let cookie; > + let i = 0; Can you rename this variable to something more specific, like `cookieCount`?

Comment on attachment 8756353 [details] [diff] [review] Part 4 - Add a test case for the session store regression test. Review of attachment 8756353 [details] [diff] [review]: ----------------------------------------------------------------- Perhaps it'd be cool to test for a cookie _with_ origin attributes here? Might be out of scope here, though. ::: browser/components/sessionstore/test/browser.ini @@ +105,5 @@ > [browser_purge_shistory.js] > skip-if = e10s # Bug 1271024 > [browser_replace_load.js] > [browser_restore_redirect.js] > +[browser_restoreSession_cookies_noOriginAttributes.js] Please rename this file to 'browser_restore_cookies_noOriginAttributes.js', since it's quite explicit already that we're testing session store. ::: browser/components/sessionstore/test/browser_restoreSession_cookies_noOriginAttributes.js @@ +38,5 @@ > + }],\ > + \"selected\": 1,\ > + \"_closedTabs\": [],\ > + \"busy\": false,\ > + \"width\": 1280,\ I *think* these dimensions are overdoing it a bit for our test systems. Can you stick to 1024x768?

Try looks good. Please ignore "interdiff_part2.patch" when check in. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/796f83f7888a https://hg.mozilla.org/integration/mozilla-inbound/rev/984443e45c69 https://hg.mozilla.org/integration/mozilla-inbound/rev/56b04484f389 https://hg.mozilla.org/integration/mozilla-inbound/rev/5f821afd7299

https://hg.mozilla.org/mozilla-central/rev/796f83f7888a https://hg.mozilla.org/mozilla-central/rev/984443e45c69 https://hg.mozilla.org/mozilla-central/rev/56b04484f389 https://hg.mozilla.org/mozilla-central/rev/5f821afd7299

Great! Good to see this bug being landed. Thanks, Tim and all the reviewers!

I'm an add-on dev and stumbled across the new parameter aOriginAttributes as there are warnings printed to the JS Console since FF 49. This parameter is completed undocumented in MDN as well as how to use it. Could you please adjust the documentation to accommodate changes of interface nsICookieManager2 and adjust the release notes for FF 49? Thanks!

I just saw another addon author confused by this deprecation warning. Tim, are you able to update the docs? (eg, https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager2#getCookiesFromHost())

There is a Bug [1] to handle the documentation of OriginAttributes. This should be updated on that Bug. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1280333

> There is a Bug [1] to handle the documentation of OriginAttributes. This should be updated on that Bug. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1280333 Why is there a separate bug to simply add the new parameters for these APIs to the devmo page? When you add or update an API, it's your responsibility to update the devmo page. This bug has been dev-doc-needed for over 8 months now.