In the file dom/base/contentAreaDropListener.js there is this code: > 84 // Use file:/// as the default uri so that drops of file URIs are always allowed > 85 let principal = sourceNode ? sourceNode.nodePrincipal > 86 : secMan.getSimpleCodebasePrincipal(ioService.newURI("file:///", null, null)); And in the file toolkit/content/nsDragAndDrop.js there is this code: > 590 var principal = sourceDoc ? sourceDoc.nodePrincipal > 591 : secMan.getSimpleCodebasePrincipal(ioService.newURI("file:///", null, null)); in both places, i think we need to change the call to getSimpleCodebasePrincipal to a createCodebasePrincipal call with an origin attribute with global user context id. in both places, the principal is used for calling SecurityManager::CheckLoadURIStrWithPrincipal to check if we can load the thing that has been dropped. in the case where sourceDoc is null, the thing being dropped came from outside the application and is treated as a "file:///" uri. i think the default user context id in that case should be the global context.

getSimpleCodebasePrincipal does the right thing.

Is this when dragging an image out of a page and on to the desktop, in order to save the image? Or is this when the user drags a HTML file or a url from the desktop to a browser-tab in order to view the file/url in the tab?

(In reply to Jonas Sicking (:sicking) from comment #2) > Or is this when the user drags a HTML file or a url from the desktop to a > browser-tab in order to view the file/url in the tab? Yes, this is the case. Also it could be dragged to another window. I assume when we drag URL to the new tab, the new tab could inherit the userContextId of the original tab.(Is this correct?) To make things more complicated, this could also drag the URL into an existing tab with different userContextId. I am going to reopen this, also ni? for huseby and tanvi for the behavior here, When we drag to a new tab, will the new tab inherit the userContextId? Should we disallow dragging URL into a differnt userContextId?

(In reply to Yoshi Huang[:allstars.chh] from comment #3) > When we drag to a new tab, will the new tab inherit the userContextId? > Should we disallow dragging URL into a differnt userContextId? This is tricky. Lots of different scenarios here: * User drags html file from the Desktop to the browser in a new tab - create a new tab with user context id = 0. * User drags html file from the Desktop to the browser in an existing tab - keep the user context id already set on that tab. * User drags url from the Desktop to the browser in a new tab - create a new tab with user context id = 0. * User drags url from the Desktop to the browser in an existing tab - keep the user context id already set on that tab. * User drags url from a document in a tab to a new tab - create a new tab with the same user context that the original tab had (inherit the context id the one the drag even came from) * User drags url form a document in a tab to an existing tab that has the same context - open the url in the context of the existing tab * User drags url form a document in a tab to an existing tab that has a different context - this is the tricky one. We want to avoid changing the usercontext id of an already existing tab. So in this case, open the new url in the context that is already assigned to the existing tab, even though it doesn't match the context of the tab it was dragged from. Are there other scenarios I missed? Is any of the above unclear? Please let me know. Also, let me know if the above is too difficult to implement (maybe not worth the code complexity) and we can try and simplify. Thanks!

(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #4) > * User drags url form a document in a tab to an existing tab that has a > different context - this is the tricky one. We want to avoid changing the > usercontext id of an already existing tab. So in this case, open the new > url in the context that is already assigned to the existing tab, even though > it doesn't match the context of the tab it was dragged from. > > Are there other scenarios I missed? Is any of the above unclear? Please > let me know. Also, let me know if the above is too difficult to implement > (maybe not worth the code complexity) and we can try and simplify. Thanks! Hi Tanvi Understood, that's pretty much what I have in mind. I don't know this component very well, will take time to check how much work is needed. For the last point, (drag into an existing tab with differnt userContextId), And FYI Recently I am working on SessionRestore for container, it has similar problem, to restore into an existing tab with different container context, But Jonas' opinion is to remove that tab and create a new one. https://bugzilla.mozilla.org/show_bug.cgi?id=1250063#c42 Just let you know there might be different behavior for different components here. I am not sure if that will confuse users. Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #5) > (In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #4) > > * User drags url form a document in a tab to an existing tab that has a > > different context - this is the tricky one. We want to avoid changing the > > usercontext id of an already existing tab. So in this case, open the new > > url in the context that is already assigned to the existing tab, even though > > it doesn't match the context of the tab it was dragged from. > > > For the last point, (drag into an existing tab with differnt userContextId), > And FYI > Recently I am working on SessionRestore for container, it has similar > problem, > to restore into an existing tab with different container context, > But Jonas' opinion is to remove that tab and create a new one. > https://bugzilla.mozilla.org/show_bug.cgi?id=1250063#c42 > > Just let you know there might be different behavior for different components > here. > I am not sure if that will confuse users. > Its a question of what the user's expectations are. Why would a user drag a url from one context into a tab that has a different context? Two situations come to mind: 1) The user no longer needs the url in the second different context tab, so uses it instead of closing that tab and opening a new one 2) The user wants to open the tab, but in a different context. The user looks across the tabs in the open window and selects the one with the right context color at the top of the tab. The user drag and drops the url into a tab of that context. For the first situation, removing the different context tab and adding a new same context tab is a better solution. For the second situation, using the existing tab is a better solution. I've never dragged and dropped urls into tabs (before tonight) so it is hard to say whether 1) or 2) above would be more common. For the session restore case, we know for sure that the user previously opened a tab in a certain context. We should hence make sure to honor it when we restore sessions. Since tabs are created before urls are restored by the SessionRestore code, and since we can't change the context id of an already existing tab, the only solution to that bug is to remove the tab and add a new one with the right context. But the situation in this bug is a little different, as the user has not previously expressed what context they would like the dragged url to be opened in. Adding Bram in case he has thoughts on this.

Here's the truth table: Target → │ Dflt │ Work │ Bank │ ↓ Source ↓ │ │ │ │ ────────────┼──────┼──────┼──────┤ Dflt │ Dflt │ Work │ Bank │ ────────────┼──────┼──────┼──────┤ Work │ Work │ Work │ Bank │ ────────────┼──────┼──────┼──────┤ Bank │ Bank │ Work │ Bank │ ────────────┴──────┴──────┴──────┘ Notes: * When dragging a link/file from the desktop, the source user context is the default context. * When dragging a link/file to the new tab button (+) the target user context is the default context. This truth table is an xor relationship for the logic test of "should we use the target user context?" If the source and target are different, the xor is true and we should take on the user context of the target. I agree with Tanvi on the session restore stuff. I also agree with Tanvi that when dragging a link from one context tab to another context tab, we should load the link the destination context. The only danger comes from tracking/session IDs in the URL itself. Maybe we should trim any GET parameters from the URL in that case?

(In reply to Dave Huseby [:huseby] from comment #7) > Here's the truth table: > > Target → │ Dflt │ Work │ Bank │ > ↓ Source ↓ │ │ │ │ > ────────────┼──────┼──────┼──────┤ > Dflt │ Dflt │ Work │ Bank │ > ────────────┼──────┼──────┼──────┤ > Work │ Work │ Work │ Bank │ > ────────────┼──────┼──────┼──────┤ > Bank │ Bank │ Work │ Bank │ > ────────────┴──────┴──────┴──────┘ > But Tanvi's Comment 4 pointed out * User drags html file from the Desktop to the browser in an existing tab - keep the user context id already set on that tab. The entries table[1][0], table[2][0] (dragging from Work/Bank into Default) are not consistent with what Tanvi says. I am not sure if the DEFAULT_USER_CONTEXT_ID could be counted as userContextId *set on that tab*. So I would like to confirm with you (Tanvi + Dave) again on this. Thanks

Hi Dave, I'll take this one if you don't mind. Thanks

Dave said Tanvi's is right during meeting. So I'll go with Tanvi's comment 6

Target → │ Dflt │ Work │ Bank │ ↓ Source ↓ │ │ │ │ ────────────┼──────┼──────┼──────┤ Dflt │ Dflt │ Work │ Bank │ ────────────┼──────┼──────┼──────┤ Work │ Dflt │ Work │ Bank │ ────────────┼──────┼──────┼──────┤ Bank │ Dflt │ Work │ Bank │ ────────────┴──────┴──────┴──────┘ Updated the truth table. If you drag from Work to Default, you will keep the Default Container. Since once a Container is set to one context, we won't change it to another context. This was a design decision that helps keep things simple. If we need to re-evaluate that in the future, we can. But for now an existing tab can't change contexts. Sorry for the delay, and please let me know if any more questions come up.

(In reply to Tanvi Vyas - behind on needinfos [:tanvi] from comment #11) > Sorry for the delay, and please let me know if any more questions come up. No worries, Dave also provided some comments during meeting, which also helps a lot. So I'll fix this into 3 parts, and writing tests for it now 1. Drag an URL into a new tab should use the userContextId of current selected tab. 2. Drag an URL into an existing tab should use the userContextId of the target tab. 3. And I'll spend sometime checking if using getSimpleCodebasePrincipal is neccesary, as UNKNOWN_APP_ID might have problems with UserContextId. (bug 1259341)

So far I found this test will have some intermitten failure (10%), still checking the test case now.

From 1d4eac7023c3c5f44a9b38b745c2e6528e691442 Mon Sep 17 00:00:00 2001 userContextId. --- browser/base/content/tabbrowser.xml | 5 ++- browser/base/content/test/general/browser.ini | 1 + .../content/test/general/browser_bug1237077.js | 47 ++++++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 browser/base/content/test/general/browser_bug1237077.js

From 66f2c9ae152b83007b635c4aee95729c08d11e5d Mon Sep 17 00:00:00 2001 --- dom/base/contentAreaDropListener.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

From 66f2c9ae152b83007b635c4aee95729c08d11e5d Mon Sep 17 00:00:00 2001 --- dom/base/contentAreaDropListener.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

From 77ad6fdaab162f17bca7a154e5f511542dbf3177 Mon Sep 17 00:00:00 2001 Drag an URL into an existing tab should inherit its userContextId. See the table from https://bugzilla.mozilla.org/show_bug.cgi?id=1237077#c11 --- .../content/test/general/browser_bug1237077.js | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+)

Comment on attachment 8742198 [details] [diff] [review] Part 3: use createCodebasePrincipal Review of attachment 8742198 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tanvi I replaced getSimpleCodebasePrincipal for some reasons. 1. getSimpleCodebase will use UNKNOWN_APP_ID, as we don't have b2g now, we should steer away using UNKNOWN_APP_ID now(also see bug 1259871) 2. In this function the principal is used to check the URI, so we could replace it with createCodebasePrincipal to make it easier.

Comment on attachment 8742199 [details] [diff] [review] Part 2: drag an URL into an existing tab ># HG changeset patch ># User Yoshi Huang <allstars.chh@mozilla.com> ># Date 1460341050 -28800 ># Mon Apr 11 10:17:30 2016 +0800 ># Node ID fd5e9325e5cc7754a1e26d7467e797921c5ca4b8 ># Parent 1bd75a25142f898353a4bcfff7a982355d461e0c >Bug 1237077 - Part 2: drag an URL into an existing tab. >From 77ad6fdaab162f17bca7a154e5f511542dbf3177 Mon Sep 17 00:00:00 2001 >Drag an URL into an existing tab should inherit its userContextId. It is unclear what "its" refers to, the tab or the url. To be clear, I would replace with: "...should inherit the existing tab's userContextId." >See the table from >https://bugzilla.mozilla.org/show_bug.cgi?id=1237077#c11 >--- > .../content/test/general/browser_bug1237077.js | 29 ++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > >diff --git a/browser/base/content/test/general/browser_bug1237077.js b/browser/base/content/test/general/browser_bug1237077.js >--- a/browser/base/content/test/general/browser_bug1237077.js >+++ b/browser/base/content/test/general/browser_bug1237077.js >@@ -40,8 +40,37 @@ add_task(function* () { > Assert.equal(content.document.documentURI, "http://test1.example.com/"); > Assert.equal(content.document.nodePrincipal.originAttributes.userContextId, 1); > }); > > yield BrowserTestUtils.removeTab(tab); > yield BrowserTestUtils.removeTab(tab2); > }); > >+/** >+ * When dragging an URL into existing tab, the existing tab should still keep >+ * the same userContextId. >+ */ Change to "When dragging a URL from one tab or link on a tab to an existing tab, the existing tab should not change its userContextId. Ex: if you drag a link from tab 1 with userContext 1 to tab 2 with userContext 2, the link will open in tab 2 with userContext 2." You also need a browser peer review.

Comment on attachment 8742198 [details] [diff] [review] Part 3: use createCodebasePrincipal What about toolkit/content/nsDragAndDrop.js. Does that need to be updated as well? I see the tests in these diffs but I don't see where the actual truth table is implemented. Perhaps it is already working the way described in comment 11 and doesn't require code changes?

(In reply to Tanvi Vyas [:tanvi] from comment #23) > Comment on attachment 8742198 [details] [diff] [review] > Part 3: use createCodebasePrincipal > > What about toolkit/content/nsDragAndDrop.js. Sorry meant to point to here: https://mxr.mozilla.org/mozilla-central/source/toolkit/content/nsDragAndDrop.js#590 > Does that need to be updated > as well? > > I see the tests in these diffs but I don't see where the actual truth table > is implemented. Perhaps it is already working the way described in comment > 11 and doesn't require code changes? Creating an brand new tab with new usercontext id is done in part 1. And reusing an existing tab must already have the right behavior. Can we ensure that referrers are not sent when we drag a url from context tab to another context tab? The general implementation for that should already be done, but we should test it works properly for these dragging cases. Thanks Yoshi!

(In reply to Tanvi Vyas [:tanvi] from comment #24) > (In reply to Tanvi Vyas [:tanvi] from comment #23) > > Comment on attachment 8742198 [details] [diff] [review] > > Part 3: use createCodebasePrincipal > > > > What about toolkit/content/nsDragAndDrop.js. > Sorry meant to point to here: > https://mxr.mozilla.org/mozilla-central/source/toolkit/content/nsDragAndDrop. > js#590 > Sorry I forgot to mention that, I didn't find any callers for nsDragAndDrop.js at the time I wrote this patch, so I didn't modify it. And in the top of that file it also mentions it's a deprecated API. https://mxr.mozilla.org/mozilla-central/source/toolkit/content/nsDragAndDrop.js#9 (The comment is added in Bug 456106, quite old) > > Can we ensure that referrers are not sent when we drag a url from context > tab to another context tab? The general implementation for that should > already be done, but we should test it works properly for these dragging > cases. Sure, will add more tests for this. Thanks

While looking at this, I also noticed the following. Shouldn't nsNullPrincipal::Create() here be passing the OriginAttributes that are passed into CreateCodebasePrincipal: http://mxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#539 https://mxr.mozilla.org/mozilla-central/source/caps/nsNullPrincipal.cpp#47 Maybe we also need to do an audit of calls to nsNullPrincipal::Create(): http://mxr.mozilla.org/mozilla-central/search?string=nsNullPrincipal%3A%3ACreate%28%29

(In reply to Tanvi Vyas [:tanvi] from comment #26) > While looking at this, I also noticed the following. Shouldn't > nsNullPrincipal::Create() here be passing the OriginAttributes that are > passed into CreateCodebasePrincipal: > http://mxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#539 > https://mxr.mozilla.org/mozilla-central/source/caps/nsNullPrincipal.cpp#47 > > Maybe we also need to do an audit of calls to nsNullPrincipal::Create(): > http://mxr.mozilla.org/mozilla-central/ > search?string=nsNullPrincipal%3A%3ACreate%28%29 Yeah, I am fixing this in Bug 1263496 now. :)

addressed tanvi's comment

Hi Tanvi Sorry for another review, I recall we need to add tags for the mochitest, so I updated browser.ini again. Since you already r+ed Part 1, so I just create another patch for this. Thanks

Comment on attachment 8742200 [details] [diff] [review] Part 1: drag an URL into new tab should inherit userContextId Review of attachment 8742200 [details] [diff] [review]: ----------------------------------------------------------------- Hi Gijs I found that when we enable the UserContextIdentity (enable privacy.userContext.enabled) I created a tab with Personal context and load some URL, then I dragger some URL into a new tab, the new tab doesn't inherit the userContextId. I found that we didn't pass the userContextId to the new tab, so I've fixed it and have a test case for this, would you review this for me? Thanks

Comment on attachment 8743109 [details] [diff] [review] Part 2: drag an URL into an existing tab v2 Review of attachment 8743109 [details] [diff] [review]: ----------------------------------------------------------------- Hi Gijs This is similar to Part 1, only that this one drags the URL into an existing tab. The original code already works, so I just wrote another test to verify this. Thanks

Comment on attachment 8743116 [details] [diff] [review] Part 4: check referrer is empty >diff --git a/browser/base/content/test/general/browser_bug1237077.js b/browser/base/content/test/general/browser_bug1237077.js >index a09900c..90ef68f 100644 >--- a/browser/base/content/test/general/browser_bug1237077.js >+++ b/browser/base/content/test/general/browser_bug1237077.js >@@ -34,16 +34,17 @@ add_task(function* () { > let tab2 = yield newTabPromise; > yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser); > > Assert.equal(tab2.getAttribute("usercontextid"), 1); > > yield ContentTask.spawn(tab2.linkedBrowser, { }, function* (args) { > Assert.equal(content.document.documentURI, "http://test1.example.com/"); > Assert.equal(content.document.nodePrincipal.originAttributes.userContextId, 1); >+ Assert.equal(content.document.referrer, "", "referrer should be empty"); I believe this is in the test described as: /** * When dragging an URL to a new tab, the new tab should have the same * userContextId with original tab. */ In this case, you are dragging from userContextId 1 to a new tab that will also get userContextId 1. So you do not need to strip the referrer. The referrer shoudl follow whatever policy the website or the user has set. Did you run this test locally? I would think that the empty referrer assertion would get triggered and fail. The referrer shoudl still be there since we are within the same usercontext.

(In reply to Tanvi Vyas [:tanvi] from comment #33) > In this case, you are dragging from userContextId 1 to a new tab that will > also get userContextId 1. So you do not need to strip the referrer. The > referrer shoudl follow whatever policy the website or the user has set. Did > you run this test locally? I would think that the empty referrer assertion > would get triggered and fail. The referrer shoudl still be there since we > are within the same usercontext. I ran the test locally and on try, both passed https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac99c2b136ec&selectedJob=19698819 I didn't do anything for the referrer. And from the code, neither using an existing tab nor creating a new tab will pass the referrer information. (even in normal tab without userContextId) https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5999 https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#6007

Comment on attachment 8742200 [details] [diff] [review] Part 1: drag an URL into new tab should inherit userContextId Review of attachment 8742200 [details] [diff] [review]: ----------------------------------------------------------------- Can you name the test file based on what it tests, rather than just a bug number reference? something like browser_usercontextid_tabdrop.js or something. I would also like you to add the test in browser/components/contextualidentity/test/browser/ instead of in browser/base/content/test/general, which is a bit of a dumping ground that I'm trying to clean up. Finally, it would be useful if you added a test to this file that tested the case where there is no usercontextid - right now your patch always passes userContextId, even if the getAttribute call returned null. It works because we check if the value you're passing is falsy here: https://dxr.mozilla.org/mozilla-central/rev/ae7413abfa4d3954a6a4ce7c1613a7100f367f9a/browser/base/content/tabbrowser.xml#1771-1773 but that seems fragile to me. Considering these changes, I would like to review the next iteration of the patch as well, please. ::: browser/base/content/test/general/browser_bug1237077.js @@ +1,3 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ Test files shouldn't have a license header and thereby should be public domain. Please add "use strict"; at the top instead. @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"]. > + getService(Ci.mozIJSSubScriptLoader); You can just use Services.scriptloader instead. @@ +8,5 @@ > +scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js", ChromeUtils); > + > +/** > + * When dragging an URL to a new tab, the new tab should have the same > + * userContextId with original tab. English nit: ... the same userContextId / as the / original tab @@ +13,5 @@ > + */ > +add_task(function* () { > + let tab = gBrowser.addTab("http://example.com/", {userContextId: 1}); > + let browser = tab.linkedBrowser; > + yield BrowserTestUtils.browserLoaded(browser); You don't use the browser variable, so you can just use BTU.browserLoaded(tab.linkedBrowser) directly. @@ +33,5 @@ > + > + let tab2 = yield newTabPromise; > + yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser); > + > + Assert.equal(tab2.getAttribute("usercontextid"), 1); Shouldn't the usercontextid be present even before the tab has loaded? That is, it seems this check should go before the yield ...browserLoaded(tab2.linkedBrowser). @@ +35,5 @@ > + yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser); > + > + Assert.equal(tab2.getAttribute("usercontextid"), 1); > + > + yield ContentTask.spawn(tab2.linkedBrowser, { }, function* (args) { Nit: you don't use args, so you can just use function*().

Comment on attachment 8743109 [details] [diff] [review] Part 2: drag an URL into an existing tab v2 Review of attachment 8743109 [details] [diff] [review]: ----------------------------------------------------------------- Same nits here as in the previous patch.

Comment on attachment 8742200 [details] [diff] [review] Part 1: drag an URL into new tab should inherit userContextId Review of attachment 8742200 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_bug1237077.js @@ +19,5 @@ > + let awaitDrop = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "drop"); > + let newTabPromise = BrowserTestUtils.waitForNewTab(gBrowser, "http://test1.example.com/"); > + > + // See explanation from browser/base/content/test/general/browser_tabDrop.js > + // for how a new tab is created. Please just copy the explanation.

(In reply to :Gijs Kruitbosch from comment #35) > Comment on attachment 8742200 [details] [diff] [review] > Part 1: drag an URL into new tab should inherit userContextId > > Review of attachment 8742200 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you name the test file based on what it tests, rather than just a bug > number reference? something like browser_usercontextid_tabdrop.js or > something. > > I would also like you to add the test in > browser/components/contextualidentity/test/browser/ instead of in > browser/base/content/test/general, which is a bit of a dumping ground that > I'm trying to clean up. > Done, move and rename it to browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js > Finally, it would be useful if you added a test to this file that tested the > case where there is no usercontextid - right now your patch always passes > userContextId, even if the getAttribute call returned null. It works because > we check if the value you're passing is falsy here: > https://dxr.mozilla.org/mozilla-central/rev/ > ae7413abfa4d3954a6a4ce7c1613a7100f367f9a/browser/base/content/tabbrowser. > xml#1771-1773 but that seems fragile to me. > I updated my patch in tabbrowser.xml let userContextId = this.selectedItem.getAttribute("usercontextid") || undefined; So userContextId is only passed when it's truly value. I've also added a test for regular tab. > > ::: browser/base/content/test/general/browser_bug1237077.js > @@ +1,3 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > Test files shouldn't have a license header and thereby should be public > domain. > Done, use Public Domain. > Please add "use strict"; at the top instead. > Done > @@ +2,5 @@ > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"]. > > + getService(Ci.mozIJSSubScriptLoader); > > You can just use Services.scriptloader instead. > Done > @@ +8,5 @@ > > +scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js", ChromeUtils); > > + > > +/** > > + * When dragging an URL to a new tab, the new tab should have the same > > + * userContextId with original tab. > > English nit: ... the same userContextId / as the / original tab > Done > @@ +13,5 @@ > > + */ > > +add_task(function* () { > > + let tab = gBrowser.addTab("http://example.com/", {userContextId: 1}); > > + let browser = tab.linkedBrowser; > > + yield BrowserTestUtils.browserLoaded(browser); > > You don't use the browser variable, so you can just use > BTU.browserLoaded(tab.linkedBrowser) directly. > Done > @@ +33,5 @@ > > + > > + let tab2 = yield newTabPromise; > > + yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser); > > + > > + Assert.equal(tab2.getAttribute("usercontextid"), 1); > > Shouldn't the usercontextid be present even before the tab has loaded? That > is, it seems this check should go before the yield > ...browserLoaded(tab2.linkedBrowser). > Done > @@ +35,5 @@ > > + yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser); > > + > > + Assert.equal(tab2.getAttribute("usercontextid"), 1); > > + > > + yield ContentTask.spawn(tab2.linkedBrowser, { }, function* (args) { > > Nit: you don't use args, so you can just use function*(). Done. Thanks for your prompt review, also I merge Part 5 into this, which adds tag for this test.

ni? tanvi for Comment 34.

Comment on attachment 8743704 [details] [diff] [review] Part 1: drag an URL into new tab should inherit userContextId v2 Review of attachment 8743704 [details] [diff] [review]: ----------------------------------------------------------------- r=me without the "|| undefined" bit in tabbrowser.xml, and with the other issues addressed. ::: browser/base/content/tabbrowser.xml @@ +5986,5 @@ > > if (!url) > return; > > + let userContextId = this.selectedItem.getAttribute("usercontextid") || undefined; I don't understand why you've added the "|| undefined" bit - getAttribute will return an empty string if the attribute is not present, and that should already trigger the correct behaviour in addTab - doesn't it? ::: browser/components/contextualidentity/test/browser/browser.ini @@ +5,5 @@ > file_reflect_cookie_into_title.html > > [browser_usercontext.js] > +[browser_usercontextid_tabdrop.js] > +tags = oa usercontext What is "oa" ? Typo? None of the other user context tests have the "usercontext" tag set, so I don't think it's useful to add it here - can you remove this line? ::: browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js @@ +1,2 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ Again, you don't need a license header. Just get rid of it altogether on this file. See this post: http://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/ @@ +31,5 @@ > + > + yield awaitDrop; > + > + let tab2 = yield newTabPromise; > + Assert.equal(tab2.getAttribute("usercontextid"), ""); Nit: Assert.ok(!tab2.hasAttribute("usercontextid"), "Tab shouldn't have usercontextid attribute");

(In reply to :Gijs Kruitbosch from comment #41) > ::: browser/base/content/tabbrowser.xml > @@ +5986,5 @@ > > > > if (!url) > > return; > > > > + let userContextId = this.selectedItem.getAttribute("usercontextid") || undefined; > > I don't understand why you've added the "|| undefined" bit - getAttribute > will return an empty string if the attribute is not present, and that should > already trigger the correct behaviour in addTab - doesn't it? > In your comment 35 you said that "right now your patch always passes userContextId, even if the getAttribute call returned null. " I thought you're referring to I always pass userContextId when calling this.tabbrowser.loadOneTab, even when the value is falsy "". But if this is not what you meant in the first place, then I'll remove the "|| undefined" part, and move it to the line before calling loadOneTab. > ::: browser/components/contextualidentity/test/browser/browser.ini > @@ +5,5 @@ > > file_reflect_cookie_into_title.html > > > > [browser_usercontext.js] > > +[browser_usercontextid_tabdrop.js] > > +tags = oa usercontext > > What is "oa" ? Short for OriginAttributes, for bugs related to origin attributes, we will tag those bugs with [OA] in white board. > None of the other user context tests have the > "usercontext" tag set, so I don't think it's useful to add it here - > Now we plan to add a tag for userContext, Bug 1264152. Sorry for not mentioned this before, but we decide to add two tags for them, oa and userContext oa is a tag for all OriginAttributes related tag, not only userContext. I asked r? to tanvi in Comment 30, she r+'ed it, so I merge it into one commit this time. > ::: > browser/components/contextualidentity/test/browser/ > browser_usercontextid_tabdrop.js > @@ +1,2 @@ > > +/* Any copyright is dedicated to the Public Domain. > > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > > Again, you don't need a license header. Just get rid of it altogether on > this file. See this post: > http://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public- > domain/ > Sorry, I misunderstood before, will remove it.

(In reply to Yoshi Huang[:allstars.chh] from comment #42) > (In reply to :Gijs Kruitbosch from comment #41) > > > ::: browser/base/content/tabbrowser.xml > > @@ +5986,5 @@ > > > > > > if (!url) > > > return; > > > > > > + let userContextId = this.selectedItem.getAttribute("usercontextid") || undefined; > > > > I don't understand why you've added the "|| undefined" bit - getAttribute > > will return an empty string if the attribute is not present, and that should > > already trigger the correct behaviour in addTab - doesn't it? > > > In your comment 35 you said that > "right now your patch always passes userContextId, even if the getAttribute > call returned null. " > I thought you're referring to I always pass userContextId when calling > this.tabbrowser.loadOneTab, even when the value is falsy "". Yes. But you're still passing the value in your current patch - it'll just be explicitly set to undefined. Run this in a console to see what I mean: let x = undefined; let y = {x}; "x" in y // returns true > But if this is not what you meant in the first place, then I'll remove the > "|| undefined" part, and move it to the line before calling loadOneTab. No, please just pass the userContextId variable as-is (which will be empty string if the tab has no usercontextid attribute) My point was that you're relying on the consumer code in addTab to ignore the ""/null/undefined value. If the consumer code did: if ('userContextId' in options) { tab.setAttribute("usercontextid", options.userContextId); } then the current code (or any other code that explicitly passes undefined as the value) is *worse* because userContextId will get coerced to a string and so we'll set the attribute to the value of "undefined". However, the consumer code doesn't currently do this, so passing either "" or undefined works. So I asked for you to add a test to ensure this continues to work correctly in future in case we make changes to the tabbrowser code and forget about this, which you duly did. That fixes the issue I originally had with the patch - the actual code doesn't need changing, I just wanted to ensure we had a test that would catch problems if the addTab code ever stopped dealing gracefully with falsy values (like if we refactor tabbrowser.xml). Does that make sense? If you now feel strongly that we should change the tabbrowser.xml code, I would probably also r+ a patch that avoided setting userContextId on the options argument to addTab if it wasn't present, but your current patch does not accomplish this - it still passes userContextId. > > ::: browser/components/contextualidentity/test/browser/browser.ini > > @@ +5,5 @@ > > > file_reflect_cookie_into_title.html > > > > > > [browser_usercontext.js] > > > +[browser_usercontextid_tabdrop.js] > > > +tags = oa usercontext > > > > What is "oa" ? > Short for OriginAttributes, for bugs related to origin attributes, we will > tag those bugs with [OA] in white board. Can we use "originattributes" instead, which would be clearer for people not familiar with the project? > > None of the other user context tests have the > > "usercontext" tag set, so I don't think it's useful to add it here - > > > Now we plan to add a tag for userContext, Bug 1264152. > > Sorry for not mentioned this before, but we decide to add two tags for them, > oa and userContext > oa is a tag for all OriginAttributes related tag, not only userContext. > I asked r? to tanvi in Comment 30, she r+'ed it, so I merge it into one > commit this time. OK, but then we should probably add it to all the tests in this file, which means we can add it under the "[DEFAULTS]" section, right?

(In reply to :Gijs Kruitbosch from comment #43) > Yes. But you're still passing the value in your current patch - it'll just > be explicitly set to undefined. Run this in a console to see what I mean: > > let x = undefined; > let y = {x}; > "x" in y // returns true > > > > But if this is not what you meant in the first place, then I'll remove the > > "|| undefined" part, and move it to the line before calling loadOneTab. > > No, please just pass the userContextId variable as-is (which will be empty > string if the tab has no usercontextid attribute) > > My point was that you're relying on the consumer code in addTab to ignore > the ""/null/undefined value. If the consumer code did: > > if ('userContextId' in options) { > tab.setAttribute("usercontextid", options.userContextId); > } > > then the current code (or any other code that explicitly passes undefined as > the value) is *worse* because userContextId will get coerced to a string and > so we'll set the attribute to the value of "undefined". > > However, the consumer code doesn't currently do this, so passing either "" > or undefined works. So I asked for you to add a test to ensure this > continues to work correctly in future in case we make changes to the > tabbrowser code and forget about this, which you duly did. That fixes the > issue I originally had with the patch - the actual code doesn't need > changing, I just wanted to ensure we had a test that would catch problems if > the addTab code ever stopped dealing gracefully with falsy values (like if > we refactor tabbrowser.xml). Does that make sense? > Understood now, really thanks for your detail explanation. I'll remove the extra || undefined. > > > > ::: browser/components/contextualidentity/test/browser/browser.ini > > > @@ +5,5 @@ > > > > file_reflect_cookie_into_title.html > > > > > > > > [browser_usercontext.js] > > > > +[browser_usercontextid_tabdrop.js] > > > > +tags = oa usercontext > > > > > > What is "oa" ? > > Short for OriginAttributes, for bugs related to origin attributes, we will > > tag those bugs with [OA] in white board. > > Can we use "originattributes" instead, which would be clearer for people not > familiar with the project? > > > > None of the other user context tests have the > > > "usercontext" tag set, so I don't think it's useful to add it here - > > > > > Now we plan to add a tag for userContext, Bug 1264152. > > > > Sorry for not mentioned this before, but we decide to add two tags for them, > > oa and userContext > > oa is a tag for all OriginAttributes related tag, not only userContext. > > I asked r? to tanvi in Comment 30, she r+'ed it, so I merge it into one > > commit this time. > > OK, but then we should probably add it to all the tests in this file, which > means we can add it under the "[DEFAULTS]" section, right? I agree that 'OA' is too short, but I guess their original intension is 'originattributes' is too long, so I think we should move the discussion to Bug 1264152 to have an agreement there, I'll also remove the tags in this patch. Thanks again for your prompt review :D

removed || undefined, license.

Comment on attachment 8744159 [details] [diff] [review] Part 1: drag an URL into new tab should inherit userContextId v3 Review of attachment 8744159 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +5994,5 @@ > > let tab = this._getDragTargetTab(event, true); > if (!tab) { > // We're adding a new tab. > + let userContextId = this.selectedItem.getAttribute("usercontextid"); also move this line to here, in v2 it was in line 5994. ::: browser/components/contextualidentity/test/browser/browser.ini @@ +4,5 @@ > empty_file.html > file_reflect_cookie_into_title.html > > [browser_usercontext.js] > +[browser_usercontextid_tabdrop.js] also forgot to mention that I also removed tag.

(In reply to Yoshi Huang[:allstars.chh] from comment #34) > (In reply to Tanvi Vyas [:tanvi] from comment #33) > > In this case, you are dragging from userContextId 1 to a new tab that will > > also get userContextId 1. So you do not need to strip the referrer. The > > referrer shoudl follow whatever policy the website or the user has set. Did > > you run this test locally? I would think that the empty referrer assertion > > would get triggered and fail. The referrer shoudl still be there since we > > are within the same usercontext. > > I ran the test locally and on try, both passed > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=ac99c2b136ec&selectedJob=19698819 > > I didn't do anything for the referrer. > And from the code, neither using an existing tab nor creating a new tab will > pass the referrer information. (even in normal tab without userContextId) > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/ > tabbrowser.xml#5999 > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/ > tabbrowser.xml#6007 Ah okay, so it looks like drag and drop tabs never pass referrers (whether you drag into an existing tab or a new tab). Just in case that changes, we should add the assertion to a test that crosses user contexts, where the first tab and the second tab have different user contexts.

(In reply to Tanvi Vyas [:tanvi] from comment #47) > Ah okay, so it looks like drag and drop tabs never pass referrers (whether > you drag into an existing tab or a new tab). Just in case that changes, we > should add the assertion to a test that crosses user contexts, where the > first tab and the second tab have different user contexts. Hi Tanvi Can you be more specifically ? Isn't my Part 4 patch doing this already but you r-'ed it? I am not sure what you meant here. Thanks

found out I missed the last nits.

rebase since the test is moved to another diretory.

Comment on attachment 8745220 [details] [diff] [review] Part 1: drag an URL into new tab should inherit userContextId v4 Hi Yoshi, I went over this test and have the following question >diff --git a/browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js b/browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js >new file mode 100644 >index 0000000..e6795f3 >--- /dev/null >+++ b/browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js >@@ -0,0 +1,85 @@ >+"use strict"; >+ >+let ChromeUtils = {}; >+Services.scriptloader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js", ChromeUtils); >+ >+/** >+ * Dragging an URL to a tab without userContextId set. >+ */ >+add_task(function* () { >+ let tab = gBrowser.addTab("http://example.com/"); This tab is added without a usercontextid. That means it will get a usercontextid of 0 correct? ... >+ let tab2 = yield newTabPromise; >+ Assert.ok(!tab2.hasAttribute("usercontextid"), "Tab shouldn't have usercontextid attribute"); In that case, the new tab should also have usercontextid = 0. Why does it have no attribute set on it?

Comment on attachment 8745221 [details] [diff] [review] Part 4: check referrer is empty. v2 >diff --git a/browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js b/browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js >index 59c6e0b..aef8def 100644 >--- a/browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js >+++ b/browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js >@@ -32,16 +32,17 @@ add_task(function* () { > let tab2 = yield newTabPromise; > Assert.ok(!tab2.hasAttribute("usercontextid"), "Tab shouldn't have usercontextid attribute"); > > yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser); > > yield ContentTask.spawn(tab2.linkedBrowser, {}, function* () { > Assert.equal(content.document.documentURI, "http://test1.example.com/"); > Assert.equal(content.document.nodePrincipal.originAttributes.userContextId, 0); Add comment: // referrer is empty when urls are dragged to new or existing tabs. // if this changes in the future, it would be okay to send the referrer // in this case because we are creating a new tab with same usercontextid // as the original tab. Depending on the answer to comment 51, the above comment may not be completely accurate. But I'd like to add something like this to future proof the test. So if referrers are turned on for dragging operations in the future, someone will have a hint on how this test should and shouldn't be updated. >@@ -73,16 +74,17 @@ add_task(function* () { > let tab2 = yield newTabPromise; > Assert.equal(tab2.getAttribute("usercontextid"), 1); > > yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser); > > yield ContentTask.spawn(tab2.linkedBrowser, {}, function* () { > Assert.equal(content.document.documentURI, "http://test1.example.com/"); > Assert.equal(content.document.nodePrincipal.originAttributes.userContextId, 1); Add comment: // referrer is empty when urls are dragged to new or existing tabs. // if this changes in the future, it would be okay to send the referrer // in this case because we are creating a new tab with same usercontextid // as the original tab. >+ Assert.equal(content.document.referrer, "", "referrer should be empty"); > }); >@@ -104,13 +106,14 @@ add_task(function* () { > yield awaitDrop; > Assert.equal(tab2.getAttribute("usercontextid"), 2); > > yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser); > > yield ContentTask.spawn(tab2.linkedBrowser, {}, function* () { > Assert.equal(content.document.documentURI, "http://test1.example.com/"); > Assert.equal(content.document.nodePrincipal.originAttributes.userContextId, 2); Add comment: // referrer is empty when urls are dragged to new or existing tabs. // if this changes in the future, we should ensure that we are not sending // a referrer for this case! When opening links across user contexts, we // don't want the referrer to follow the user from one context to another. >+ Assert.equal(content.document.referrer, "", "referrer should be empty"); > });

(In reply to Tanvi Vyas [:tanvi] from comment #51) > >+/** > >+ * Dragging an URL to a tab without userContextId set. > >+ */ > >+add_task(function* () { > >+ let tab = gBrowser.addTab("http://example.com/"); > This tab is added without a usercontextid. That means it will get a > usercontextid of 0 correct? This is the default, but not represented on the tab as an attribute. > >+ let tab2 = yield newTabPromise; > >+ Assert.ok(!tab2.hasAttribute("usercontextid"), "Tab shouldn't have usercontextid attribute"); > > In that case, the new tab should also have usercontextid = 0. Why does it > have no attribute set on it? So the same happens here - no attribute is set, AIUI.

(In reply to :Gijs Kruitbosch from comment #53) > (In reply to Tanvi Vyas [:tanvi] from comment #51) > > >+/** > > >+ * Dragging an URL to a tab without userContextId set. > > >+ */ > > >+add_task(function* () { > > >+ let tab = gBrowser.addTab("http://example.com/"); > > This tab is added without a usercontextid. That means it will get a > > usercontextid of 0 correct? > > This is the default, but not represented on the tab as an attribute. > > > >+ let tab2 = yield newTabPromise; > > >+ Assert.ok(!tab2.hasAttribute("usercontextid"), "Tab shouldn't have usercontextid attribute"); > > > > In that case, the new tab should also have usercontextid = 0. Why does it > > have no attribute set on it? > > So the same happens here - no attribute is set, AIUI. Ah okay. Thanks Gijs!

(In reply to Tanvi Vyas [:tanvi] from comment #52) > > > yield ContentTask.spawn(tab2.linkedBrowser, {}, function* () { > > Assert.equal(content.document.documentURI, "http://test1.example.com/"); > > Assert.equal(content.document.nodePrincipal.originAttributes.userContextId, 0); > > Add comment: > // referrer is empty when urls are dragged to new or existing tabs. > // if this changes in the future, it would be okay to send the referrer > // in this case because we are creating a new tab with same usercontextid > // as the original tab. > I'll re-words the last part, 'with *default* usercontextid as the original tab.' > > Assert.equal(content.document.documentURI, "http://test1.example.com/"); > > Assert.equal(content.document.nodePrincipal.originAttributes.userContextId, 1); > Add comment: > // referrer is empty when urls are dragged to new or existing tabs. > // if this changes in the future, it would be okay to send the referrer > // in this case because we are creating a new tab with same usercontextid > // as the original tab. 'with *the* same usercontextid as the original tab.'

addressed tanvi's comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c569dc7fca70 https://hg.mozilla.org/integration/mozilla-inbound/rev/5cbd332e6cfa https://hg.mozilla.org/integration/mozilla-inbound/rev/804144abc693 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee93ef1f7ed

https://hg.mozilla.org/mozilla-central/rev/c569dc7fca70 https://hg.mozilla.org/mozilla-central/rev/5cbd332e6cfa https://hg.mozilla.org/mozilla-central/rev/804144abc693 https://hg.mozilla.org/mozilla-central/rev/3ee93ef1f7ed