1260595 - All items in Themes panel are duplicated if I exit customize with the panel opened (should close/empty panel when leaving customize mode)

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160326030430
STR:
1. Right-click Australis Menu button (≡), click "Customize..."
2. Click "Themes" button at the bottom
3. Press Ctrl+T
4. Switch to Customize tab, click "Themes" button again

AR:  All items in Themes panel are duplicated
ER:  Items shouldn't be duplicated

Comment 1

[arni2033]

Attachment: screenshot 1 - All items in Themes panel are duplicated.png

Comment 2

[:Gijs (he/him)]

This is a regression, right? I expect this is similar in cause and/or maybe even a dupe of bug 1258344

Comment 3

[arni2033]

I tested Release in advance. Also, it's not necessary to press Ctrl+T in Step 3. Ctrl+W is enough.
If you leaved Customize with the panel opened, then the next time you open panel, you see the bug.

So the only chance for this to be fixed by 1258344 is scenario when a code is so bad that any thoughtful intervention fixes several bugs  (I can afford this joke today)

Comment 4

[:Gijs (he/him)]

(In reply to arni2033 from comment #3)
> If you leaved Customize with the panel opened, then the next time you open
> panel, you see the bug.

It was not obvious to me that the panel was left open when switching away from the tab - it doesn't take much to close it, and so I could not reproduce. Thanks for clarifying.

Jared, maybe this is also something one of our Outreachy folks could look at?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, good idea. I've flagged it to show up in my buglist.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

This should be a good next bug for you to work on Katie. Let me know if you have any questions.

Comment 7

[Katie Broida [:ktbee]]

Sounds good!

Comment 8

[Katie Broida [:ktbee]]

Attachment: Patch to clear Themes menu before it's populated.

Taking a look at this, I found the Theme menu options were duplicated because they had never been cleared by onLWThemesMenuHidden(), which is triggered by the onpopuphidden event. If the popup is left open, but the tab is closed, the menu items are left in the panel when the tab is re-opened. To make sure the menu items are cleared before the menu appears, I've edited the function that populates the menu ( onLWThemesMenuShowing() ) to call onLWThemesMenuHidden() before it populates its menu items.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8741838 [details] [diff] [review]
Patch to clear Themes menu before it's populated.

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

It would be good to have a test that confirms this works and keeps it working in the future. You might be able to look at /browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js.

::: browser/components/customizableui/CustomizeMode.jsm
@@ +1341,5 @@
>    onLWThemesMenuShowing: function(aEvent) {
>      const DEFAULT_THEME_ID = "{972ce4c6-7e08-4474-a285-3208198ce6fd}";
>      const RECENT_LWT_COUNT = 5;
>  
> +    this.onLWThemesMenuHidden(aEvent);

Can you rename this function to resetLWThemesMenu(), and change the argument so it doesn't expect an event, but the event.target?

Like so,
resetLWThemesMenu(target) {
  let doc = target.ownerDocument;
  ...
}

Comment 10

[arni2033]

Why doesn't 'popuphidden' event fire? Well, _that_ is the real bug. And modifying functions is a hack
However, such hack is still a very good thing if somebody will also file the underlying bug
If somebody agrees with me and has a better understanding of XUL events, please file it (or OK, make it clear for me that I'll need to debug it myself)

Comment 11

[Katie Broida [:ktbee]]

Attachment: V2 of patch to clear Theme menu items

Thank you for the feedback, Jared. I've gone ahead and made those changes with this patch version.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8742474 [details] [diff] [review]
V2 of patch to clear Theme menu items

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

This looks good but we should have a test that confirms this works and keeps it working in the future. You might be able to add to /browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js.

Comment 13

[Katie Broida [:ktbee]]

Attachment: V3 with test for duplicate Themes in menu

Makes sense! I've added a test in this patch that checks the Themes panel after opening the Customize menu, closing the menu without exiting the panel popup, and then re-opening the Customize menu.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8743311 [details] [diff] [review]
V3 with test for duplicate Themes in menu

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

This is very close but I noticed a subtle bug below.

::: browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
@@ +28,5 @@
> +  yield startCustomizing();
> +  info("Started customizing a second time");
> +  yield EventUtils.synthesizeMouseAtCenter(themesButton, {});
> +  info("Clicked on themes button a second time");
> +  yield popupShownPromise;

You will need to get assign a new promise from popupShown to popupShownPromise as the one that you are referring to here is still resolved as of earlier. Just add `popupShownPromise = popupShown(popup);` before your call to syntheziseMouseAtCenter.

Comment 15

[Katie Broida [:ktbee]]

Attachment: V4 fixes popupShownPromise bug in test

Thanks for catching that, Jared. I've reset popupShownPromise in this patch as you suggested.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8743869 [details] [diff] [review]
V4 fixes popupShownPromise bug in test

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

::: browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
@@ +27,5 @@
> +  info("Exited customize mode");
> +  yield startCustomizing();
> +  info("Started customizing a second time");
> +  popupShownPromise = popupShown(popup);
> +  yield EventUtils.synthesizeMouseAtCenter(themesButton, {});

I didn't notice this the other time, but we don't need to yield on EventUtils.synthesizeMouseAtCenter. I'll fix it for you.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in

Comment 18

[Pulsebot]

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

Comment 19

[:Gijs (he/him)]

(In reply to Pulsebot from comment #18)
> https://hg.mozilla.org/integration/fx-team/rev/c7544b24989d
Sorry, I had to back this out, because Linux really didn't like this:

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=4fa5909cf1e73939ee823442f2c69b0a86474489&selectedJob=8899431

 21:14:00     INFO -  Thread 0 (crashed)
 21:14:00     INFO -   0  libxul.so!nsWindow::OnExposeEvent [nsWindow.cpp:4fa5909cf1e7 : 2316 + 0xc]
 21:14:00     INFO -      eip = 0xb3f655e9   esp = 0xbfcd54c0   ebp = 0xbfcd55a8   ebx = 0xb61e3344
 21:14:00     INFO -      esi = 0xbfcd5574   edi = 0x99d27220   eax = 0xe5e5e5e5   ecx = 0xbfcd5394
 21:14:00     INFO -      edx = 0xbfcd5398   efl = 0x00210292
 21:14:00     INFO -      Found by: given as instruction pointer in context
 21:14:00     INFO -   1  libxul.so!draw_window_of_widget [nsWindow.cpp:4fa5909cf1e7 : 5471 + 0xd]
 21:14:00     INFO -      eip = 0xb3f65b9e   esp = 0xbfcd55b0   ebp = 0xbfcd55f8   ebx = 0xb61e3344
 21:14:00     INFO -      esi = 0x8d013150   edi = 0xbfcd55dc
 21:14:00     INFO -      Found by: call frame info
 21:14:00     INFO -   2  libxul.so!expose_event_cb [nsWindow.cpp:4fa5909cf1e7 : 5494 + 0x16]
 21:14:00     INFO -      eip = 0xb3f65c4d   esp = 0xbfcd5600   ebp = 0xbfcd5618   ebx = 0xb61e3344
 21:14:00     INFO -      esi = 0xb7297070   edi = 0xbfcd57a4
 21:14:00     INFO -      Found by: call frame info
 21:14:00     INFO -   3  libgtk-3.so.0.400.1!_gtk_marshal_BOOLEAN__BOXED [gtkmarshalers.c : 85 + 0x10]
 21:14:00     INFO -      eip = 0xb6c8d022   esp = 0xbfcd5620   ebp = 0xbfcd5810   ebx = 0xb6fbeff4
 21:14:00     INFO -      esi = 0xb3f65c24   edi = 0xbfcd57a4
 21:14:00     INFO -      Found by: call frame info
 21:14:00     INFO -   4  0xb7297070
 21:14:00     INFO -      eip = 0xb7297070   esp = 0xbfcd5818   ebp = 0xa25203c0
 21:14:00     INFO -      Found by: previous frame's frame pointer


Don't know if that's going to be locally-reproducible or not, but it was so permafail after this run that it might be, that it seems not-unlikely.

https://hg.mozilla.org/integration/fx-team/rev/005449bf12df

This looks like a gtk3 issue rather than directly the fault of the patch - Jared, maybe you can have a look?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Gijs, it looks like this crash is related to the tab closing while the menu is still populating (see bug 1269392 which I might dupe over).

I'm able to fix this test by putting a 1 second pause after the first popupshown promise is resolved, however using waitForCondition(recommendedHeader.nextSibling != footer) doesn't fix the crash.

Are you OK with the 1 second pause? (yield new Promise(setTimeout(resolve, 1000));)

Comment 21

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Gijs, it looks like this crash is related to the tab closing while the menu
> is still populating (see bug 1269392 which I might dupe over).

If you fire a custom event when the menu finishes populating and listen for that in the test, does that fix the issue?

> I'm able to fix this test by putting a 1 second pause after the first
> popupshown promise is resolved, however using
> waitForCondition(recommendedHeader.nextSibling != footer) doesn't fix the
> crash.
> 
> Are you OK with the 1 second pause? (yield new Promise(setTimeout(resolve,
> 1000));)

Shouldn't we fix the underlying crash in widget-land instead?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> > Gijs, it looks like this crash is related to the tab closing while the menu
> > is still populating (see bug 1269392 which I might dupe over).
> 
> If you fire a custom event when the menu finishes populating and listen for
> that in the test, does that fix the issue?

That is effectively the waitForCondition that I mentioned in comment 20. 
> 
> > I'm able to fix this test by putting a 1 second pause after the first
> > popupshown promise is resolved, however using
> > waitForCondition(recommendedHeader.nextSibling != footer) doesn't fix the
> > crash.
> > 
> > Are you OK with the 1 second pause? (yield new Promise(setTimeout(resolve,
> > 1000));)
> 
> Shouldn't we fix the underlying crash in widget-land instead?

Yeah, we should, though I haven't had more time to investigate it yet and likely won't in the near future. I'll file a followup for the crash and land this with the test disabled on Linux (OK'd by Gijs over IRC).

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in (test disabled on Linux)

Comment 24

[Pulsebot]

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cee62dcbbef8

Comment 26

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 48.0a1 on windows 7, 64 bit!

This bug's fix is verified on latest Developer Edition,Nightly.


Build ID   20160708004052
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

Build ID   20160714030208
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

But i could not find the fix in Beta 48.0b8