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

VERIFIED FIXED in Firefox 49

Status

()

VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: arni2033, Assigned: ktbee, Mentored)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 49
Points:
3
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 affected, firefox47 affected, firefox48 affected, firefox49 verified, firefox-esr38 affected)

Details

(Whiteboard: [outreachy-12])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

3 years ago
Created attachment 8736078 [details]
screenshot 1 - All items in Themes panel are duplicated.png

Comment 2

3 years ago
This is a regression, right? I expect this is similar in cause and/or maybe even a dupe of bug 1258344
Flags: needinfo?(arni2033)
(Reporter)

Comment 3

3 years ago
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)
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox-esr38: --- → affected
Flags: needinfo?(arni2033)
Summary: All items in Themes panel are duplicated if I exit customize with Ctrl+T and then enter it again → All items in Themes panel are duplicated if I exit customize with the panel opened

Comment 4

3 years ago
(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?
Flags: needinfo?(jaws)
Summary: All items in Themes panel are duplicated if I exit customize with the panel opened → All items in Themes panel are duplicated if I exit customize with the panel opened (should close/empty panel when leaving customize mode)
Yeah, good idea. I've flagged it to show up in my buglist.
Mentor: jaws
Points: --- → 3
Flags: needinfo?(jaws)
Whiteboard: [outreachy-12]
This should be a good next bug for you to work on Katie. Let me know if you have any questions.
Assignee: nobody → kbroida
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 years ago
Sounds good!
(Assignee)

Comment 8

3 years ago
Created attachment 8741838 [details] [diff] [review]
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.
Attachment #8741838 - Flags: review?(jaws)
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;
  ...
}
Attachment #8741838 - Flags: review?(jaws) → review-
(Reporter)

Comment 10

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

Comment 11

3 years ago
Created attachment 8742474 [details] [diff] [review]
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.
Attachment #8741838 - Attachment is obsolete: true
Attachment #8742474 - Flags: review?(jaws)
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.
Attachment #8742474 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 13

3 years ago
Created attachment 8743311 [details] [diff] [review]
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.
Attachment #8742474 - Attachment is obsolete: true
Attachment #8743311 - Flags: review?(jaws)
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.
Attachment #8743311 - Flags: review?(jaws) → review-
(Assignee)

Comment 15

3 years ago
Created attachment 8743869 [details] [diff] [review]
V4 fixes popupShownPromise bug in test

Thanks for catching that, Jared. I've reset popupShownPromise in this patch as you suggested.
Attachment #8743311 - Attachment is obsolete: true
Attachment #8743869 - Flags: review?(jaws)
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.
Attachment #8743869 - Flags: review?(jaws) → review+

Comment 19

3 years ago
(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?
Flags: needinfo?(jaws)
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));)
Flags: needinfo?(jaws) → needinfo?(gijskruitbosch+bugs)

Comment 21

3 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jaws)
(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).
Flags: needinfo?(jaws)
Created attachment 8750835 [details] [diff] [review]
Patch for check-in (test disabled on Linux)
Attachment #8743951 - Attachment is obsolete: true

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cee62dcbbef8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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
status-firefox49: fixed → verified

Updated

2 years ago
Depends on: 1306584
(Reporter)

Updated

2 years ago
See Also: → bug 1327797
You need to log in before you can comment on or make changes to this bug.