1251841 - New instances of about:preferences open when I click Sync buttons in Menu, Menu bar, Homepage, Toolbar and normal Options buttons

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P3)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 47, 32bit, ID 20160225030209
STR:
0. Move Searchbar, "Options" button, "Synced tabs" button to toolbar
1. Open http://example.org/ in a new tab
2. Click "Synced tabs" button in toolbar, click "Sign in to Sync"
3. Click Australis Menu button (≡) -> "Sign in to Sync"
4. Press F10 to open Menu bar, click "Tools" -> "Sign in to Sync"
5.A) Click dropmarker in Searchbar, click "Change Search Settings"
5.B) Type "a" in urlbar, click "Change Search Settings" in urlbar suggestions
5.C) Click "Options" button in toolbar

AR:  Each step (2-5) about:preferences page opens in a new tab. Eventually there're 4 options tabs
ER:  Either X or Y
 X) A new Preferences tab should open every time I open preferences due to bug 1208222, bug 1208208
 Y) After performing Steps 0-5 there should be only 1 instance of about:preferences

Comment 1

[arni2033]

Hm, probably it's better to treat  5.A as "step 5",  5.B as "step 6",  5.C as "step 7"
i.e. perform 7 steps to see the inconsistency better.
In this case, you'll see how Steps 5-7 load new url in existing tab, while Steps 2-4 open new tabs

Comment 2

[Chris Karlof [:ckarlof]]

Each of these actions opens a new prefs URL with slightly different parameters. E.g., the Sync ones differ by the entrypoint param. I believe each action tries to reuse an existing tab with exactly the same URL but will open a new one when the params differ slightly.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Most of these calls end up at https://dxr.mozilla.org/mozilla-central/rev/9da51cb4974e03cdd8fa45a34086fe1033abfeaf/browser/base/content/utilityOverlay.js#567. The function that calls is defined at https://dxr.mozilla.org/mozilla-central/rev/9da51cb4974e03cdd8fa45a34086fe1033abfeaf/browser/base/content/browser.js#7335 and the "options" param accepts a replaceQueryString flag. This flag would solve the problem, and I can't think of any reason it can't be unconditionally specified when opening preferences.

Comment 4

[arni2033]

6. Open about:home, click "Sync" button on that page

Comment 5

[Kurt Carpenter]

Hello, I'd like to attempt this bug as a first contribution. I'll do my best to have a patch in by the end of this weekend.

Thanks for pointing out the replaceQueryString flag Mark, I'll give that a try.

Comment 6

[Kurt Carpenter]

Attachment: fix.bug1251841.patch

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8764465 [details] [diff] [review]
fix.bug1251841.patch

I'll have a look at this soon.

Comment 8

[Kurt Carpenter]

Attachment: fix.bug1251841.patch (Amended commit message)

Figured you might want the commit message reformatted. Sorry for the many CC notifs

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8765740 [details] [diff] [review]
fix.bug1251841.patch (Amended commit message)

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

Thanks for the patch - at a minimum it will require new tests in  browser_bug1025195_switchToTabHavingURI_aOpenParams.js .

However, this code already sucks in many ways, and I'm not particularly happy with increasing that complexity - which isn't a fault of this patch, but this patch does offer the opportunity to fix that with a little more work.  ie, it's quite difficult to see in this patch how all the flags relate to each other, and difficult to see if the 3 different places where we reuse a tab all have the same logic.

I think we should explore something like:

* Use "new URL()" on the passed-in string and the URL for each window we find, and use its methods to "normalize" the URLs based on the flags before checking if they match.
* Use url.searchParams object to replace the query string if necessary (rather than a regex)
* Use a single loop handling all cases so there's exactly one place where we switch to an existing one (possibly reloading)
* Add this new combination of flags to the test mentioned above.

Feel free to ping me on IRC if you'd like to chat more.

Comment 10

[Kurt Carpenter]

Attachment: fix - reorganized code + tests

Restructured the code, added comments, added some tests

Comment 11

[Kurt Carpenter]

Attachment: fix - reorganized code, added tests

Accidentally committed a couple debugging statements last time.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8766929 [details] [diff] [review]
fix - reorganized code, added tests

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

I think this patch still needs the utilityOverlay.js change from your first patch to actually fix this bug - please add that back and test the actual bug as reported is fixed.

But this is looking great, and almost all comments are nits - thanks! Please upload a new version with the comments fixed and I think we will be good to go.

::: browser/base/content/browser.js
@@ +7419,5 @@
>        return false;
>      }
> +    /**
> +     * Remove the query string, fragment, both, or neither from a URL string.
> +     * @param URIstring

I think you can remove the @param and @return descriptions here, given it is an internal helper.

@@ +7427,5 @@
> +     * @param removeFragment
> +     *        boolean set to 'true' to remove the fragment
> +     * @return URL string with the designated components removed
> +     */
> +    function cleanURL(URIstring, removeQuery, removeFragment) {

There's a vague convention that things name "URI" are objects, while "URL" means a string - so I think changing "URIstring" to simply, say, "url" is fine.

@@ +7446,5 @@
> +    }
> +
> +    // Need to handle nsSimpleURIs here too (e.g. about:...), which don't
> +    // work correctly with URL objects - so treat them as strings
> +    let aCompare = cleanURL(

Things with a leading "a" are args - I think you should rename this to, say, requestedCompare and browserCompare. I think folding this into a single line is also OK even though it will go beyond 80 chars (similarly below)

@@ +7452,3 @@
>      let browsers = aWindow.gBrowser.browsers;
>      for (let i = 0; i < browsers.length; i++) {
> +      let browser = browsers[i]

you removed the trailing ; on this line.

@@ +7453,5 @@
>      for (let i = 0; i < browsers.length; i++) {
> +      let browser = browsers[i]
> +      let bCompare = cleanURL(
> +          browser.currentURI.spec, ignoreQueryString || replaceQueryString, ignoreFragment);
> +      if (aCompare === bCompare) {

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style says to prefer == over ===

::: browser/base/content/test/urlbar/browser_bug1025195_switchToTabHavingURI_aOpenParams.js
@@ +72,5 @@
>    yield promiseTabLoaded(tabRefAboutHome);
>    is(gBrowser.currentURI.spec, "about:home?hello=firefoxos", "The spec should be updated to the new spec");
>    cleanupTestTabs();
>  });
> +add_task(function test_replaceQueryStringAndFragment() {

add a newline here.

Note also that all of these functions are generators, so should be declared as:

|add_task(function* test_replaceQueryStringAndFragment() {|

When this test was initially written that syntax wasn't supported, but I think we might as well fix that while we are touching this file - so please change all the existing tests to also use this new syntax (ie, add that "*" character to all the existing add_task() calls) and to all new ones you add.

Comment 13

[Kurt Carpenter]

Attachment: Amended fix

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8767338 [details] [diff] [review]
Amended fix

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

That looks great, thanks!

Comment 15

[Carsten Book [:Tomcat]]

has problems to apply:

applying fix.bug1251841.patch
patching file browser/base/content/test/urlbar/browser_bug1025195_switchToTabHavingURI_aOpenParams.js
Hunk #1 FAILED at 0
Hunk #2 FAILED at 29
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh fix.bug1251841.patch

Comment 16

[Chris Karlof [:ckarlof]]

Mark, could you look into what's needed to get this to apply?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/94968a940273
switchIFURIInWindow can ignore fragments AND query strings, add tests. r=markh

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94968a940273

Comment 19

[Azmina]

I have reproduced this bug with Nightly 47.0a1 (2016-02-27) , on Windows 8.1 , 64 bit!

This bug's fix is verified on Latest Aurora 50.0a2.

Build ID : 20160804004004
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20160803]

Comment 20

[Rezaul Huque Nayeem]

Successfully reproduced this bug in firefox nightly 47.0a1 (2016-02-27) as comment 0 with ubuntu 16.04 (64 bit)

Verified this bug as fixed with latest firefox aurora 50.0a2 (Build ID: 20160804004004)
Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

As this bug is also verified on Windows (comment 19), Marking this bug as verified!