Closed Bug 974481 Opened 6 years ago Closed 6 years ago

Quickly Exiting Customize Mode after Restoring Defaults Makes Customize Sticky/Broken

Categories

(Firefox :: Toolbars and Customization, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: shorlander, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2])

Attachments

(3 files, 1 obsolete file)

Attached image Broken Mode Example
STR:

1) Customize your toolbar
2) Exit Customize
3) Return to Customize
4) Press "Restore Defaults" and very quickly move to and press the "Exit Customize" button
5) Aspects of your chrome should be stuck in Customize Mode e.g. greyed out locationbar

There is a visible lag while the tools palette populates. It seems if you can exit before this finishes it will trigger this.

There isn't a good way to recovering from this besides closing and reopening the window. Re-entering and exiting will fix some elements but population of the tools field will sometimes double and eventually the window will break completely.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
See Also: → 973153
Attached patch PatchSplinter Review
I had a hard time writing a test for this because there are lots of exceptions that come from not yielding for resetCustomization mode to finish. Are you OK with this landing without a test?
Attachment #8381920 - Flags: review?(gijskruitbosch+bugs)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Created attachment 8381920 [details] [diff] [review]
> Patch
> 
> I had a hard time writing a test for this because there are lots of
> exceptions that come from not yielding for resetCustomization mode to
> finish. Are you OK with this landing without a test?

Wait, why are there lots of exceptions? What kind of exceptions?
Comment on attachment 8381920 [details] [diff] [review]
Patch

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

I'm OK with landing without a test, but I'd like to understand what the exceptions are... this code looks pretty straightforward...
Attachment #8381920 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch 974481test.patch (obsolete) — Splinter Review
(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> > Created attachment 8381920 [details] [diff] [review]
> > Patch
> > 
> > I had a hard time writing a test for this because there are lots of
> > exceptions that come from not yielding for resetCustomization mode to
> > finish. Are you OK with this landing without a test?
> 
> Wait, why are there lots of exceptions? What kind of exceptions?

The exceptions come from running the test without the changes applied. They go away with the changes applied. If our test infra failed due to exceptions being thrown in a Task then we would be OK, but they don't (bug 976205). I'd like the test to simply check the parentNode of the urlbar-container to see that it is not a toolbarpaletteitem, but unfortunately that test passes without the patch applied. I've attached the test here to see what you think. We could land it anyways and once bug 976205 lands then we will be covered.
Attachment #8382277 - Flags: review?(gijskruitbosch+bugs)
Attached patch TestSplinter Review
Attachment #8382277 - Attachment is obsolete: true
Attachment #8382277 - Flags: review?(gijskruitbosch+bugs)
Attachment #8382278 - Flags: review?(gijskruitbosch+bugs)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Created attachment 8382278 [details] [diff] [review]
> Test

Instead of this, can't you get the promise returned by gCustomizeMode.reset() and check if it's rejected after endCustomizing() returns ?
As discussed on IRC, it's not that simple since the error condition of a promise is only hit once, not returned through the chain of `then` calls.

Pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/1ac1efebb5a5
Attachment #8382278 - Flags: review?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/1ac1efebb5a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment on attachment 8381920 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): initial implementation of 'restore defaults'
User impact if declined: quickly clicking buttons can leave the UI in an unusable state
Testing completed (on m-c, etc.): locally, just landed on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #8381920 - Flags: approval-mozilla-aurora?
Attachment #8381920 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: cornel.ionce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0

Verified as fixed on Latest Aurora (build ID: 20140306004001) and on latest Nightly (build ID: 20140306030201).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.