985777 - Unable to exit customize if normal and private customize windows are open

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[James Kitchener (:jkitch)]

Steps to reproduce.

1. With a normal Nightly window open, open a new private window.
2. Enter Customize mode on each window.
3. In one of the windows move something onto the navigation toolbar (say developer tools).
4. Click restore defaults in that window.
5. Try to exit customize mode in both windows.

Expected results:
Both windows should exit customize mode and return to their previous open tab.
(Alternatively, only one window should be able to enter customize mode if the inability to open customize in multiple normal windows is correct).

Actual Result:
"Exit Customize" does nothing in one of the windows (probably the one where "Restore Defaults" was pressed).

Comment 1

[:Gijs (he/him)]

(In reply to James Kitchener (:jkitch) from comment #0)
> (Alternatively, only one window should be able to enter customize mode if
> the inability to open customize in multiple normal windows is correct).

This.

Comment 2

[Dão Gottwald [:dao]]

https://hg.mozilla.org/mozilla-central/rev/57be4b233874 changed "the behavior of switchToTabHavingURI to make sure that switch to tab actually fails if somehow attempted (this is used in the test.)" We should probably just revert that part.

Comment 3

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #2)
> https://hg.mozilla.org/mozilla-central/rev/57be4b233874 changed "the
> behavior of switchToTabHavingURI to make sure that switch to tab actually
> fails if somehow attempted (this is used in the test.)" We should probably
> just revert that part.

Maybe instead we could specialcase about: URIs? I'm a little wary of changing existing behaviour (from 9 versions ago) on beta right now...

Comment 4

[:Gijs (he/him)]

Attachment: specialcase about: URIs so switching to them is allowed even to/from private windows,

So this is what I was thinking. I'll do a separate patch to just back out as you suggested in a sec - please review-/+ appropriately. :-)

Comment 5

[:Gijs (he/him)]

Attachment: always allow internal switchToTabHavingURI to switch to other windows, including private ones,

I've verified that neither of these patches break the existing test that was landed in bug 816527.

Comment 6

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/74dacf9ea7d4

Comment 7

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

Backed out in https://hg.mozilla.org/integration/fx-team/rev/f2c21183e206 for test failures in browser_bug816527.js

https://tbpl.mozilla.org/php/getParsedLog.php?id=36473308&tree=Fx-Team

Comment 8

[:Gijs (he/him)]

(In reply to Wes Kocher (:KWierso) from comment #7)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/f2c21183e206
> for test failures in browser_bug816527.js
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36473308&tree=Fx-Team

Ugh. Of course, the test works when used standalone, but not when running the entire directory. For whatever reason.

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Wes Kocher (:KWierso) from comment #7)
> > Backed out in https://hg.mozilla.org/integration/fx-team/rev/f2c21183e206
> > for test failures in browser_bug816527.js
> > 
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=36473308&tree=Fx-Team
> 
> Ugh. Of course, the test works when used standalone, but not when running
> the entire directory. For whatever reason.

So, even running just the one test before it makes things work. I don't understand any of this. I also don't understand why the test only checks if you manually type in a moz-action: url, and then hit enter, which presumably nobody actually does. Seems to me like it should check whether these items show up in the autocomplete popup at all. Ehsan, can you clarify?

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > (In reply to Wes Kocher (:KWierso) from comment #7)
> > > Backed out in https://hg.mozilla.org/integration/fx-team/rev/f2c21183e206
> > > for test failures in browser_bug816527.js
> > > 
> > > https://tbpl.mozilla.org/php/getParsedLog.php?id=36473308&tree=Fx-Team
> > 
> > Ugh. Of course, the test works when used standalone, but not when running
> > the entire directory. For whatever reason.
> 
> So, even running just the one test before it makes things work.

To be clear, that means that with the patch applied, the test:
- passes when run standalone
- fails when run with 1 test run before it

Comment 11

[(no longer active)]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > (In reply to Wes Kocher (:KWierso) from comment #7)
> > > Backed out in https://hg.mozilla.org/integration/fx-team/rev/f2c21183e206
> > > for test failures in browser_bug816527.js
> > > 
> > > https://tbpl.mozilla.org/php/getParsedLog.php?id=36473308&tree=Fx-Team
> > 
> > Ugh. Of course, the test works when used standalone, but not when running
> > the entire directory. For whatever reason.
> 
> So, even running just the one test before it makes things work. I don't
> understand any of this. I also don't understand why the test only checks if
> you manually type in a moz-action: url, and then hit enter, which presumably
> nobody actually does. Seems to me like it should check whether these items
> show up in the autocomplete popup at all. Ehsan, can you clarify?

As the commit message which added that test explains <http://hg.mozilla.org/mozilla-central/rev/57be4b233874>, this is based on what http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug555767.js does.

The moz-action URL is used to test the switch to tab behavior.  When you select a switch to tab suggestion, we set the location bar to a moz-action URL (hiding the moz-action bit from the user) which is later on processed to perform a switch to tab action as opposed to a normal navigation.  This test wants to ensure that a moz-action URL will be unable to switch to a tab which is open in a window with the wrong private browsing status, so testing what would appear in the UI would be uninteresting here.

FWIW, this patch https://hg.mozilla.org/integration/fx-team/rev/74dacf9ea7d4 is definitely expected to break this test, since it's the wrong thing to do, and I'm glad that this test caught that.

I hope this explains things, but I'm not 100% sure what question you were asking from me so I don't know if I did answer it here.  If I have not, please ni? me again!

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #11)
> This test wants to ensure that a moz-action URL will be unable to switch to a tab
> which is open in a window with the wrong private browsing status, so testing
> what would appear in the UI would be uninteresting here.

The test could also just ensure that moz-action URLs aren't produced for tabs in private windows, which is what the rest of the patch in bug 816527 does. That matches the primary user-driven goal in bug 816527.

The switchToTabHavingURI change in bug 816527 was just "unneeded additional safety", and it's breaking this particular code's expectation about that function's behavior. You can argue that the code shouldn't have that expectation, but its use case is valid.

I suppose we can "fix" this by making the block of code in question conditional on a parameter passed to switchToTabHavingURI. I don't feel strongly about which should be the default behavior, but I suppose it's better to err on the side of caution and keep the current one (post bug 816527).

Comment 13

[(no longer active)]

I disagree that the extra check was additional unneeded safety.  I discussed this extensively on IRC with Gijs, and this should be considered the PB module owner's opinion.

One other way of fixing this would be to add an optional argument in aOpenParams to suppress this check for about:customize.  But I think that opens the door to misusing this API needlessly.  I think a better solution would be to have a whitelist of URLs for which we suppress this check.  This way we get to review those URLs to ensure that ignoring this check for them wouldn't violate the PB semantics, and we can start off by adding about:customize to that whitelist (if there are additional similar ones, let's discuss them here too.)

Comment 14

[:Gijs (he/him)]

Attachment: add a whitelist for URLs that we can switch to/from private browsing windows,

I believe certain things (like about:accounts and about:addons) could maybe be added here, but I'd rather first have something we can uplift without too much ado.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

nit: Make that whitelist a set, and use .has() instead of indexOf?

Comment 16

[(no longer active)]

Comment on attachment 8394900 [details] [diff] [review]
add a whitelist for URLs that we can switch to/from private browsing windows,

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

about:accounts seems to accept some query string arguments, so in the future we might decide to do some kind of string prefix matching here.  That would mean that Gavin's suggestion won't work for that case.  I asked Gavin about this on IRC and he said we can worry about that in the future, which sounds fine to me.  So, r=me with Gavin's nit addressed.

Thanks!

Comment 17

[:Gijs (he/him)]

w/ nits:

https://hg.mozilla.org/integration/fx-team/rev/473cb1d6c18f

Ehsan, do you want me to file a separate bug about the test? AFAICT (a) it never breaks if run on its own (which is what I did to test my original patch here, which then broke the tree when it landed because the test was run with other stuff running before it), and (b) it doesn't test whether items show up in the autocomplete for the URL bar, which means if someone broke that code (rather than the switchToTabHavingURI bits that I broke), there'd be a "Switch to tab" item in the autocomplete, but that item wouldn't do anything (because of the switchToTabHavingURI code stopping it). Maybe there's already a separate test for (b)? I don't know.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8394900 [details] [diff] [review]
add a whitelist for URLs that we can switch to/from private browsing windows,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: Customize Mode can be open in two windows, which leads to all kinds of bugs
Testing completed (on m-c, etc.): local, soon on m-c
Risk to taking this patch (and alternatives if risky): low, specific exemption for about:customizing in the switchToTabHavingURI code
String or IDL/UUID changes made by this patch: none

Comment 19

[(no longer active)]

(In reply to :Gijs Kruitbosch from comment #17)
> Ehsan, do you want me to file a separate bug about the test? AFAICT (a) it
> never breaks if run on its own (which is what I did to test my original
> patch here, which then broke the tree when it landed because the test was
> run with other stuff running before it), and (b) it doesn't test whether
> items show up in the autocomplete for the URL bar, which means if someone
> broke that code (rather than the switchToTabHavingURI bits that I broke),
> there'd be a "Switch to tab" item in the autocomplete, but that item
> wouldn't do anything (because of the switchToTabHavingURI code stopping it).
> Maybe there's already a separate test for (b)? I don't know.

Yes, please file two bugs for each one of those issues.  FWIW the reason I didn't test what is shown up in the UI was I did not have a good way of testing it.  Ideas welcome.

Thanks!

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/473cb1d6c18f

Comment 21

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #19)
> (In reply to :Gijs Kruitbosch from comment #17)
> > Ehsan, do you want me to file a separate bug about the test? AFAICT (a) it
> > never breaks if run on its own (which is what I did to test my original
> > patch here, which then broke the tree when it landed because the test was
> > run with other stuff running before it), and (b) it doesn't test whether
> > items show up in the autocomplete for the URL bar, which means if someone
> > broke that code (rather than the switchToTabHavingURI bits that I broke),
> > there'd be a "Switch to tab" item in the autocomplete, but that item
> > wouldn't do anything (because of the switchToTabHavingURI code stopping it).
> > Maybe there's already a separate test for (b)? I don't know.
> 
> Yes, please file two bugs for each one of those issues.  FWIW the reason I
> didn't test what is shown up in the UI was I did not have a good way of
> testing it.  Ideas welcome.
> 
> Thanks!

I'll file these after some sleep, setting needinfo so I don't forget. Thanks!

Comment 22

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bb0f6acbdc3
https://hg.mozilla.org/releases/mozilla-beta/rev/ff06fd595c1e

Comment 23

[Andrei Vaida [:avaida]]

This issue is verified fixed on the latest Nightly (Build ID: 20140324030203), latest Aurora (Build ID: 20140324150430) and latest Beta (Build ID: 20140324101726), using:
- Windows 7 64-bit [1],
- Windows 8.1 64-bit [2],
- Ubuntu 13.10 64-bit [3],
- Mac OS X 10.9 [4].


1. Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:31.0) Gecko/20100101 Firefox/31.0
2. Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
3. Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
4. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0