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

Status: RESOLVED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

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.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1378794 - Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed.

Review commit: https://reviewboard.mozilla.org/r/154938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154938/

Comment 2

[:Gijs (he/him)]

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.

Comment 3

[Mike de Boer [:mikedeboer]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Issue also affects browser_981418-widget-onbeforecreated-handler.js:
https://treeherder.mozilla.org/logviewer.html#?job_id=112280609&repo=autoland

Comment 7

[Mike de Boer [:mikedeboer]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154938/diff/1-2/

Comment 8

[Mike de Boer [:mikedeboer]]

Sorry about that, I clearly hadn't thought about the non-Photon case. Running the fix through Try now.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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