Closed Bug 1287866 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jhao, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][domsecurity-backlog])

Attachments

(1 file, 3 obsolete files)

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.
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!
Priority: -- → P2
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?
Summary: Awesome bar switchs me to a tab in a different container. → Awesomebar shouldn't force switching to a tab in a different container.
Bram, can you provide your thoughts on this?
Flags: needinfo?(bram)
Priority: P2 → P1
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.
(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.
Priority: P1 → P3
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-backlog1]
Priority: P3 → P2
Whiteboard: [userContextId][domsecurity-backlog1] → [userContextId][domsecurity-*]
Whiteboard: [userContextId][domsecurity-*] → [userContextId][domsecurity-backlog]
Increasing the priority here since this is quite an annoying bug and could cause users to accidentally use the wrong container.
Priority: P2 → P1
Assignee: nobody → amarchesini
Attached patch patch (obsolete) — Splinter Review
Attachment #8809065 - Flags: review?(mak77)
Attachment #8809065 - Flags: review?(gijskruitbosch+bugs)
Attachment #8809065 - Attachment description: part 1 - userContextId in queries → patch
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?
Attachment #8809065 - Flags: review?(gijskruitbosch+bugs) → review-
> 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.
Attached patch search.patch (obsolete) — Splinter Review
Attachment #8809065 - Attachment is obsolete: true
Attachment #8809065 - Flags: review?(mak77)
Attachment #8809315 - Flags: review?(mak77)
Attachment #8809315 - Flags: review?(gijskruitbosch+bugs)
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) ?
Attachment #8809315 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch search.patch (obsolete) — Splinter Review
Attachment #8809315 - Attachment is obsolete: true
Attachment #8809315 - Flags: review?(mak77)
Flags: needinfo?(bram)
Attachment #8809372 - Flags: review?(mak77)
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
Attachment #8809372 - Flags: review?(mak77)
Attached patch search.patchSplinter Review
Attachment #8809372 - Attachment is obsolete: true
Attachment #8809454 - Flags: review?(mak77)
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
Attachment #8809454 - Flags: review?(mak77) → review+
Status: NEW → ASSIGNED
Pushed by amarchesini@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/89ab24bcc71f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1473977
You need to log in before you can comment on or make changes to this bug.