Closed Bug 985777 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- verified
firefox30 --- verified
firefox31 --- verified

People

(Reporter: jkitch, Assigned: Gijs)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file, 2 obsolete files)

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).
(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.
Whiteboard: [Australis:P3]
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.
Blocks: 816527
OS: Windows 7 → All
Hardware: x86_64 → All
(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...
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. :-)
Attachment #8394127 - Flags: review?(dao)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
I've verified that neither of these patches break the existing test that was landed in bug 816527.
Attachment #8394131 - Flags: review?(dao)
Attachment #8394131 - Flags: review?(dao) → review+
Attachment #8394127 - Attachment is obsolete: true
Attachment #8394127 - Flags: review?(dao)
remote:   https://hg.mozilla.org/integration/fx-team/rev/74dacf9ea7d4
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
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
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
(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.
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ehsan)
(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
(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!
Flags: needinfo?(ehsan)
(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).
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.)
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.
Attachment #8394900 - Flags: review?(ehsan)
nit: Make that whitelist a set, and use .has() instead of indexOf?
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!
Attachment #8394900 - Flags: review?(ehsan) → review+
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.
Flags: needinfo?(ehsan)
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Attachment #8394131 - Attachment is obsolete: true
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
Attachment #8394900 - Flags: approval-mozilla-beta?
Attachment #8394900 - Flags: approval-mozilla-aurora?
(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!
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/473cb1d6c18f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
(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!
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8394900 - Flags: approval-mozilla-beta?
Attachment #8394900 - Flags: approval-mozilla-beta+
Attachment #8394900 - Flags: approval-mozilla-aurora?
Attachment #8394900 - Flags: approval-mozilla-aurora+
Depends on: 987119
Depends on: 987125
Flags: needinfo?(gijskruitbosch+bugs)
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
You need to log in before you can comment on or make changes to this bug.