Closed Bug 1251841 Opened 4 years ago Closed 3 years ago

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


(Firefox :: Sync, defect, P3, minor)




Firefox 50
Tracking Status
firefox47 --- affected
firefox50 --- verified


(Reporter: arni2033, Assigned: kurtcarpenter)


(Whiteboard: [good first bug][lang=js])


(1 file, 4 obsolete files)

>>>   My Info:   Win7_64, Nightly 47, 32bit, ID 20160225030209
0. Move Searchbar, "Options" button, "Synced tabs" button to toolbar
1. Open 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
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
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.
Flags: firefox-backlog+
Priority: -- → P3
Most of these calls end up at The function that calls is defined at 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.
Whiteboard: [good first bug][lang=js]
6. Open about:home, click "Sync" button on that page
Summary: New instances of Options page (about:preferences) open when I click buttons in Menu, Menu bar, Sync and Searchbar → New instances of about:preferences open when I click Sync buttons in Menu, Menu bar, Homepage, Toolbar and normal Options buttons
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.
Attached patch fix.bug1251841.patch (obsolete) — Splinter Review
Comment on attachment 8764465 [details] [diff] [review]

I'll have a look at this soon.
Attachment #8764465 - Flags: review?(markh)
Figured you might want the commit message reformatted. Sorry for the many CC notifs
Attachment #8764465 - Attachment is obsolete: true
Attachment #8764465 - Flags: review?(markh)
Attachment #8765740 - Flags: review?(markh)
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.
Attachment #8765740 - Flags: review?(markh) → feedback+
Attached patch fix - reorganized code + tests (obsolete) — Splinter Review
Restructured the code, added comments, added some tests
Attachment #8765740 - Attachment is obsolete: true
Attachment #8766907 - Flags: review?(markh)
Accidentally committed a couple debugging statements last time.
Attachment #8766907 - Attachment is obsolete: true
Attachment #8766907 - Flags: review?(markh)
Attachment #8766929 - Flags: review?(markh)
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) { 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.
Attachment #8766929 - Flags: review?(markh)
Attached patch Amended fixSplinter Review
Attachment #8766929 - Attachment is obsolete: true
Attachment #8767338 - Flags: review?(markh)
Comment on attachment 8767338 [details] [diff] [review]
Amended fix

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

That looks great, thanks!
Attachment #8767338 - Flags: review?(markh) → review+
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
Flags: needinfo?(kurtcarpenter)
Keywords: checkin-needed
Mark, could you look into what's needed to get this to apply?
Flags: needinfo?(markh)
Pushed by
switchIFURIInWindow can ignore fragments AND query strings, add tests. r=markh
Flags: needinfo?(kurtcarpenter)
Flags: needinfo?(markh)
Assignee: nobody → kurtcarpenter
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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

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!
QA Whiteboard: [bugday-20160803]
You need to log in before you can comment on or make changes to this bug.