1287866 - Awesomebar shouldn't force switching to a tab in a different container.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Jonathan Hao (inactive) [:jhao]]

I have a tab in default container on https://misuse.co/containers/cookies.html. And then I want to open that page in the Personal container, so I opened a new container tab. When I type "misuse" in the awesome bar, an auto-complete of "https://misuse.co/containers/cookies.html" shows up, but if I click that, I'm forced to switch to the one in default container. I have to type the entire url.

Comment 1

[Tanvi Vyas[:tanvi]]

This is because of the awesomebar switch to tab feature.  We should not offer the switch to tab option if the same url is open in a tab from a different container type.

i.e.

Go to google.com in Personal Container.
Now open Work Container.  Start typing google.com. Awesomebar should not display the option to switch to the google.com Personal Tab.

Thanks for reporting Jonathan!

Comment 2

[Albert Scheiner [:alberts]]

Just a thought: Could the "Switch to tab" row be second in line after the " - Visit" / " - Search with ..." row for this case and color coded it with the different container type color?

Comment 3

[Tanvi Vyas[:tanvi]]

Bram, can you provide your thoughts on this?

Comment 4

[Jonathan Hao (inactive) [:jhao]]

The workaround I would personally use is to press the down button to get the auto completed url and type an extra space after it. This way I can open the url in the new container instead of being switched to the existing one.

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to Jonathan Hao [:jhao] from comment #4)
> The workaround I would personally use is to press the down button to get the
> auto completed url and type an extra space after it. This way I can open the
> url in the new container instead of being switched to the existing one.

Yes, I often use a url fragment (append# to the end) to get around this.  I often use it outside of containers, where I just want to open the page again instead of switching to the other tab (which might be in a different window) and messing up my work flow.  I wish there was a hot key or something that would make this easier to bypass in general.

Comment 6

[Tanvi Vyas[:tanvi]]

Increasing the priority here since this is quite an annoying bug and could cause users to accidentally use the wrong container.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 8

[:Gijs (he/him)]

Comment on attachment 8809065 [details] [diff] [review]
patch

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

r- because of the issue with duplication across usercontextids and how we store the data on the JS side.

::: browser/base/content/tabbrowser.xml
@@ +797,5 @@
>  
>                  let unifiedComplete = this.mTabBrowser._unifiedComplete;
>                  if (this.mBrowser.registeredOpenURI) {
> +                  unifiedComplete.unregisterOpenPage(this.mBrowser.registeredOpenURI,
> +                                                     this.mBrowser.hasAttribute("usercontextid")

Wasn't there a way to get this property directly, IIRC off contentPrincipal or something like that?

@@ +798,5 @@
>                  let unifiedComplete = this.mTabBrowser._unifiedComplete;
>                  if (this.mBrowser.registeredOpenURI) {
> +                  unifiedComplete.unregisterOpenPage(this.mBrowser.registeredOpenURI,
> +                                                     this.mBrowser.hasAttribute("usercontextid")
> +                                                       ? this.mBrowser.getAttribute("usercontextid")

This will be a string and the fallback is an int. Should make it consistent, I guess. Also feels like we should just factor it out into a local var a bit earlier:

let userContextId = 0;
if (...hasAttr) {
  userContextId = ...;
}


would make this easier to read and not such a long line.

::: toolkit/components/places/UnifiedComplete.js
@@ +336,2 @@
>      if (!this._conn) {
> +      this._queue.set(uri, userContextId);

This is wrong. We will only have 1 user context id per uri, but the uri could be open in tabs with multiple usercontextids.

We want a Set behaviour over the URIs per contextid. So the logical thing to do is to have a map from contextid to set, and have the set contain the URIs.

@@ +360,5 @@
>        `UPDATE moz_openpages_temp
>         SET open_count = open_count - 1
> +       WHERE url = :url
> +       AND userContextId = :userContextId`
> +    , { url: uri.spec, userContextId: userContextId });

in es6, don't need to duplicate "key: key", here and above.

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ +288,5 @@
>  function addOpenPages(aUri, aCount=1) {
>    let ac = Cc["@mozilla.org/autocomplete/search;1?name=unifiedcomplete"]
>               .getService(Ci.mozIPlacesAutoComplete);
>    for (let i = 0; i < aCount; i++) {
> +    ac.registerOpenPage(aUri, 0 /* userContextId */);

Add this as a param with the default of 0, and add a test?

Comment 9

[Andrea Marchesini [:baku]]

> Wasn't there a way to get this property directly, IIRC off contentPrincipal
> or something like that?

We cannot do that because if the content is about:newtab (or another about:page) contentPrincipal is system and its originAttributes.userContextId is always 0.

All the other comments will be applied in the next version of the patch.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: search.patch

Comment 11

[:Gijs (he/him)]

Comment on attachment 8809315 [details] [diff] [review]
search.patch

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

r=me but I'd like mak to have a look as well, esp. at the sql stuff.

::: browser/base/content/tabbrowser.xml
@@ +2583,5 @@
>              if (browser.registeredOpenURI && !aAdoptedByTab) {
> +              this._unifiedComplete.unregisterOpenPage(browser.registeredOpenURI,
> +                                                       browser.hasAttribute("usercontextid")
> +                                                         ? browser.getAttribute("usercontextid")
> +                                                         : 0);

I'm tempted to suggest:

browser.getAttribute("usercontextid") || 0

instead.

::: toolkit/components/places/UnifiedComplete.js
@@ +334,5 @@
>      // ...then clear it to avoid double additions.
>      this._queue.clear();
>    }),
>  
> +  add: function (uri, userContextId) {

If we're modifying all these method signatures, maybe this is a good opportunity to switch this to add(foo, bar) style method declarations?

@@ +361,3 @@
>      if (!this._conn) {
> +      // This should not happen.
> +      if (!this._queue.has(userContextId)) {

Should we log an error message if it does happen?

::: toolkit/components/places/mozIPlacesAutoComplete.idl
@@ +116,5 @@
>     *
>     * @param aURI
>     *        The URI to register as an open page.
>     */
> +  void registerOpenPage(in nsIURI aURI, in int32_t aUserContextId);

Isn't this a uint?

::: toolkit/components/places/tests/unifiedcomplete/test_tab_matches.js
@@ +51,5 @@
> +               makeSwitchToTabMatch("http://xyz.net/", { title: "xyz.net - we're better than ABC" }),
> +               { uri: uri5, title: "foobar.org - much better than ABC, definitely better than XYZ", style: [ "favicon" ] } ]
> +  });
> +
> +  do_print("a container tab isnot visible in 'switch to tab'");

Nit: space between "is" and "not".

Can we also add a test for what happens when checking autocomplete in a container with the 3 usercontextid (where the new thing you're adding shows up and the abc.com/xyz.net matches aren't) ?

Comment 12

[Andrea Marchesini [:baku]]

Attachment: search.patch

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8809372 [details] [diff] [review]
search.patch

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

::: browser/base/content/tabbrowser.xml
@@ +797,5 @@
>  
> +                let userContextId = 0;
> +                if (this.mBrowser.hasAttribute("usercontextid")) {
> +                  userContextId = this.mBrowser.getAttribute("usercontextid");
> +                }

nit: sounds like the new sport is to suggest alternatives for has/getAttribute, so I suggest:
let userContextId = Number(this.mBrowser.getAttribute("usercontextid")); that magically gives 0 if the attribute is not set. The same could be used below instead of browser.getAttribute("usercontextid") || 0!

Regardless, and more seriously, let's use the same method here and below, for coherency. I don't care which method.

::: toolkit/components/places/UnifiedComplete.js
@@ +166,5 @@
>     ) AS i
>     JOIN moz_places h ON h.id = i.place_id
>     LEFT JOIN moz_favicons f ON f.id = h.favicon_id
>     LEFT JOIN moz_openpages_temp t ON t.url = h.url
> +   WHERE t.userContextId = :userContextId

This looks wrong?
The adaptive entry should be returned regardless since it's in history, but not as a switch to tab entry, imo.
I'd rather move the condition to the join.

@@ +301,4 @@
>    initDatabase: Task.async(function* (conn) {
>      // To reduce IO use an in-memory table for switch-to-tab tracking.
>      // Note: this should be kept up-to-date with the definition in
>      //       nsPlacesTables.h.

I think you missed this note. It's not critical, just that we keep a collection of all the table definitions in a single place.

@@ +311,4 @@
>         )`);
>  
>      // Note: this should be kept up-to-date with the definition in
>      //       nsPlacesTriggers.h.

ditto

@@ +318,5 @@
>         WHEN NEW.open_count = 0
>         BEGIN
>           DELETE FROM moz_openpages_temp
> +         WHERE url = NEW.url
> +         AND userContextId = NEW.userContextId;

nit: please align the end of "AND" with the end of "WHERE" (yes, we use fancy indentation)

@@ +324,5 @@
>  
>      this._conn = conn;
>  
>      // Populate the table with the current cache contents...
> +    this._queue.forEach((uris, userContextId) => {

I'd prefer a
for (let [userContextId, uris] of this._queue) {
since originally Set/Map were not iterable, but now they are and for...of is more readable than the callback and does not suffer of bind problems. Indeed I'm not sure "this" in the "this.add" call is correct, the original code was passing this as a scope to forEach. this code path is not well tested and it could have gone unnoticed

@@ +325,5 @@
>      this._conn = conn;
>  
>      // Populate the table with the current cache contents...
> +    this._queue.forEach((uris, userContextId) => {
> +      uris.forEach((uri) => {

same here, for (let uri of uris)

@@ +356,2 @@
>                  )`
> +    , { url: uri.spec, userContextId: userContextId });

per ES6, just ", userContextId }"

@@ +361,4 @@
>      if (!this._conn) {
> +      // This should not happen.
> +      if (!this._queue.has(userContextId)) {
> +        throw "Unknown userContextId!";

throw new Error(...

::: toolkit/components/places/mozIPlacesAutoComplete.idl
@@ +116,5 @@
>     *
>     * @param aURI
>     *        The URI to register as an open page.
>     */
> +  void registerOpenPage(in nsIURI aURI, in uint32_t aUserContextId);

please document the @param

@@ +129,5 @@
>     *
>     * @param aURI
>     *        The URI to unregister as an open page.
>     */
> +  void unregisterOpenPage(in nsIURI aURI, in uint32_t aUserContextId);

ditto

Comment 14

[Andrea Marchesini [:baku]]

Attachment: search.patch

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8809454 [details] [diff] [review]
search.patch

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

LGTM, r=me with a green Try run!

::: toolkit/components/places/UnifiedComplete.js
@@ +167,5 @@
>     JOIN moz_places h ON h.id = i.place_id
>     LEFT JOIN moz_favicons f ON f.id = h.favicon_id
> +   LEFT JOIN moz_openpages_temp t
> +     ON t.url = h.url
> +    AND t.userContextId = :userContextId

very very nit: I'd align with the JOIN rather than with the LEFT.

@@ +319,5 @@
>         WHEN NEW.open_count = 0
>         BEGIN
>           DELETE FROM moz_openpages_temp
> +         WHERE url = NEW.url
> +         AND userContextId = NEW.userContextId;

very very nit: align end of AND with end of WHERE

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89ab24bcc71f
Awesomebar shouldn't force switching to a tab in a different container, r=Gijs, r=mak

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/89ab24bcc71f

Comment 18

[Luís Miguel [:quicksaver] (not reading bugmail)]

Hope you can give me a little pointer...

> 363 	// This should not happen.
> 364   if (!this._queue.has(userContextId)) {
> 365 	  throw new Error("Unknown userContextId!");
> 366 	}

Why not? initDatabase() clears |this._queue| all the time.

I use something like the following to "deinitialize" the unifiedComplete module (for my add-on):
> for(let browser of gBrowser.browsers) {
>   if(browser.registeredOpenURI) {
>     gBrowser._unifiedComplete.unregisterOpenPage(
>       browser.registeredOpenURI,
>       browser.getAttribute("usercontextid") || 0
>     );
>   }
> }

I hit this error when disabling my custom unifiedComplete module (which is a complete duplicate of the original in central in regards to switch-to-tab and containers code, including this patch) when I run that routine above, but I don't hit it when I disable the original unifiedComplete in exactly the same way. I have no clue why...

Any ideas? Note that I can easily get around it by simply returning instead of throwing at that point (this doesn't hit on every tab, about 4-5 tabs out of ~200, seems 100% harmless when errors hit though). But still it's weird, so I thought I'd ask. :)

Comment 19

[Luís Miguel [:quicksaver] (not reading bugmail)]

Figured it out. Never mind. Sorry about the bug spam.