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

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Sync
P3
minor
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: arni2033, Assigned: Kurt Carpenter)

Tracking

Trunk
Firefox 50
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox47 affected, firefox50 verified)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
>>>   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
(Reporter)

Comment 1

2 years ago
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 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.
Whiteboard: [good first bug][lang=js]
(Reporter)

Comment 4

2 years ago
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
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
Created attachment 8764465 [details] [diff] [review]
fix.bug1251841.patch
Comment on attachment 8764465 [details] [diff] [review]
fix.bug1251841.patch

I'll have a look at this soon.
Attachment #8764465 - Flags: review?(markh)
(Assignee)

Comment 8

2 years ago
Created attachment 8765740 [details] [diff] [review]
fix.bug1251841.patch (Amended commit message)

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+
(Assignee)

Comment 10

2 years ago
Created attachment 8766907 [details] [diff] [review]
fix - reorganized code + tests

Restructured the code, added comments, added some tests
Attachment #8765740 - Attachment is obsolete: true
Attachment #8766907 - Flags: review?(markh)
(Assignee)

Comment 11

2 years ago
Created attachment 8766929 [details] [diff] [review]
fix - reorganized code, added tests

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) {

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.
Attachment #8766929 - Flags: review?(markh)
(Assignee)

Comment 13

2 years ago
Created attachment 8767338 [details] [diff] [review]
Amended fix
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+

Updated

2 years ago
Keywords: checkin-needed
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)

Comment 17

2 years ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/fx-team/rev/94968a940273
switchIFURIInWindow can ignore fragments AND query strings, add tests. r=markh

Updated

2 years ago
Flags: needinfo?(kurtcarpenter)

Updated

2 years ago
Flags: needinfo?(markh)

Updated

2 years ago
Assignee: nobody → kurtcarpenter

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/94968a940273
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Comment 19

2 years ago
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

2 years ago
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!
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160803]
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.