When temporary panels are destroyed, the ViewHide event may be dispatched on the wrong panelview

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 56
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
I noticed this issue whilst working intensively with temporary panels in bug 1354532: the 'ViewHide' event is always dispatched on the `viewNode` that was set in the very beginning, but meanwhile another subview may have already been shown in the panel before it's destroyed.
This causes the `viewNode` to receive the 'ViewHide' event for the second time and the current viewpanel to never receive it.
Flags: qe-verify-
Comment hidden (mozreview-request)

Comment 2

2 months ago
mozreview-review
Comment on attachment 8883957 [details]
Bug 1378794 - Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed.

https://reviewboard.mozilla.org/r/154938/#review159930

::: browser/components/customizableui/content/panelUI.js:538
(Diff revision 1)
> -          viewNode.dispatchEvent(evt);
> +          let evt = new CustomEvent("ViewHiding", {detail: currentView});
> +          currentView.dispatchEvent(evt);

Doesn't the panelmultiview code do this itself? Seems like that would make more sense, but rs=me to make it not do the wrong thing, at least.
Attachment #8883957 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 3

2 months ago
mozreview-review-reply
Comment on attachment 8883957 [details]
Bug 1378794 - Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed.

https://reviewboard.mozilla.org/r/154938/#review159930

> Doesn't the panelmultiview code do this itself? Seems like that would make more sense, but rs=me to make it not do the wrong thing, at least.

Perhaps, but this may have been put here way back when (Australis era) because relying on the XBL destructor was too unreliable?

Comment 4

2 months ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f3fc2ea88f60
Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed. r=Gijs
Backed out for failing browser/components/extensions/test/browser/browser_ext_popup_corners.js:

https://hg.mozilla.org/integration/autoland/rev/8d3213b771bda968d122e0c5544470c1b4f60f02

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f3fc2ea88f607ce82ed46e329d44350bfb9c8ce2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112281980&repo=autoland

[task 2017-07-06T16:04:58.763459Z] 16:04:58     INFO - Test stand-alone browserAction popup
[task 2017-07-06T16:04:58.769672Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Expect widget not to be overflowed - 
[task 2017-07-06T16:04:58.771861Z] 16:04:58     INFO - Buffered messages logged at 16:04:57
[task 2017-07-06T16:04:58.774915Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderTopLeftRadius should be the same - 
[task 2017-07-06T16:04:58.778158Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderTopLeftRadius should be the same - 
[task 2017-07-06T16:04:58.781913Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderTopRightRadius should be the same - 
[task 2017-07-06T16:04:58.784199Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderTopRightRadius should be the same - 
[task 2017-07-06T16:04:58.786514Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderBottomRightRadius should be the same - 
[task 2017-07-06T16:04:58.790143Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderBottomRightRadius should be the same - 
[task 2017-07-06T16:04:58.793237Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderBottomLeftRadius should be the same - 
[task 2017-07-06T16:04:58.796786Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderBottomLeftRadius should be the same - 
[task 2017-07-06T16:04:58.801484Z] 16:04:58     INFO - Buffered messages finished
[task 2017-07-06T16:04:58.806170Z] 16:04:58     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_popup_corners.js | uncaught exception - TypeError: currentView is undefined at panelRemover@chrome://browser/content/customizableui/panelUI.js:539:11
[task 2017-07-06T16:04:58.808166Z] 16:04:58     INFO - hidePopup@chrome://global/content/bindings/popup.xml:137:13
[task 2017-07-06T16:04:58.810279Z] 16:04:58     INFO - hidePanelForNode@resource:///modules/CustomizableUI.jsm:1689:7
[task 2017-07-06T16:04:58.813496Z] 16:04:58     INFO - hidePanelForNode@resource:///modules/CustomizableUI.jsm:3693:5
[task 2017-07-06T16:04:58.816649Z] 16:04:58     INFO - closeBrowserAction@chrome://mochitests/content/browser/browser/components/extensions/test/browser/head.js:263:3
[task 2017-07-06T16:04:58.818701Z] 16:04:58     INFO - testPopupBorderRadius@chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_popup_corners.js:75:11
[task 2017-07-06T16:04:58.820739Z] 16:04:58     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:763:21
[task 2017-07-06T16:04:58.827555Z] 16:04:58     INFO - TaskImpl_run@resource://gre/modules/Task.jsm:331:42
[task 2017-07-06T16:04:58.829731Z] 16:04:58     INFO - TaskImpl@resource://gre/modules/Task.jsm:280:3
[task 2017-07-06T16:04:58.832860Z] 16:04:58     INFO - asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-07-06T16:04:58.834751Z] 16:04:58     INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-07-06T16:04:58.836942Z] 16:04:58     INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:758:9
[task 2017-07-06T16:04:58.843018Z] 16:04:58     INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:670:7
[task 2017-07-06T16:04:58.845479Z] 16:04:58     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2017-07-06T16:04:58.851450Z] 16:04:58     INFO - 
[task 2017-07-06T16:04:58.853398Z] 16:04:58     INFO - Stack trace:
[task 2017-07-06T16:04:58.855216Z] 16:04:58     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
[task 2017-07-06T16:04:58.858131Z] 16:04:58     INFO - chrome://global/content/bindings/popup.xml:hidePopup:137
[task 2017-07-06T16:04:58.860047Z] 16:04:58     INFO - resource:///modules/CustomizableUI.jsm:hidePanelForNode:1689
[task 2017-07-06T16:04:58.861865Z] 16:04:58     INFO - resource:///modules/CustomizableUI.jsm:hidePanelForNode:3693
[task 2017-07-06T16:04:58.864455Z] 16:04:58     INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/head.js:closeBrowserAction:263
[task 2017-07-06T16:04:58.866494Z] 16:04:58     INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_popup_corners.js:testPopupBorderRadius:75
[task 2017-07-06T16:04:58.872553Z] 16:04:58     INFO - GECKO(1092) | JavaScript error: chrome://browser/content/customizableui/panelUI.js, line 539: TypeError: currentView is undefined
Flags: needinfo?(mdeboer)
Issue also affects browser_981418-widget-onbeforecreated-handler.js:
https://treeherder.mozilla.org/logviewer.html#?job_id=112280609&repo=autoland
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 months ago
Sorry about that, I clearly hadn't thought about the non-Photon case. Running the fix through Try now.
Flags: needinfo?(mdeboer)

Comment 9

2 months ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a041f3412d61
Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed. r=Gijs

Comment 10

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a041f3412d61
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.