Closed Bug 1415036 Opened 2 years ago Closed 2 years ago

Fix Promise/Event handling in browser/base/content/test/general/browser_bug553455.js for bug 1193394

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file)

test_tabNavigate in browser/base/content/test/general/browser_bug553455.js checks if the number of pending install s become 0 after the notification closes, but the callbacks for the following are called from the same onLocationChange event:
  * notification close (closePromise there)
  * removing pending install (results in getInstalls() become 0)

and notification close happens first, and it checks the number of pending installs before it becomes 0.

I think this is the testcase's issue about what to wait for.
So far, what I observed is here.

https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/browser/base/content/test/general/browser_bug553455.js#136-143
> function waitForNotificationClose() {
>   return new Promise(resolve => {
>     info("Waiting for notification to close");
>     PopupNotifications.panel.addEventListener("popuphidden", function() {
>       resolve();
>     }, {once: true});
>   });
> }

https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/browser/base/content/test/general/browser_bug553455.js#671-677
> async function test_tabNavigate() {
> ...
>   let closePromise = waitForNotificationClose();
>   let loadPromise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
>   gBrowser.loadURI("about:blank");
>   await closePromise;
> 
>   let installs = await getInstalls();
>   is(installs.length, 0, "Should be no pending install");
> ...

https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/toolkit/content/widgets/popup.xml#149
>       <method name="hidePopup">
>         <parameter name="cancel"/>
>         <body>
>         <![CDATA[
> ...
>           else if (popupBox instanceof PopupBoxObject)
>             popupBox.hidePopup(cancel);
>         ]]>
>         </body>
>       </method>

popuphidden event listener in waitForNotificationClose gets called inside popupBox.hidePopup in popup.xml, that is native function.
while hiding popup inside popupBox.hidePopup, the closePromise is resolved, and before leaving popupBox.hidePopup function, microtask checkpoint seems to be performed,
and test_tabNavigate gets resumed, and the check about the pending installs fails.
then, the pending installs are actually removed inside another onLocationChange listener than one for tabbrowser.xml that hides popup.

_callProgressListeners @ RemoteWebProgress.jsm:181:11
 +- onLocationChange @ tabbrowser.xml:967:17
 | +- callProgressListeners @ tabbrowser.xml:582:22
 |   +- callProgressListeners @ tabbrowser.xml:494:13
 |    +- callListeners @ tabbrowser.xml:479:24
 |      +- onLocationChange @ browser.js:4637:7
 |        +- URLBarSetURI @ browser.js:2741:3
 |          +- SetPageProxyState @ browser.js:2874:3
 |            +- UpdatePopupNotificationsVisibility @ browser.js:2887:3
 |              +- anchorVisibilityChange @ PopupNotifications.jsm:587:7
 |                +- PopupNotifications_update @ PopupNotifications.jsm:1101:9
 |                  +- PopupNotifications_showPanel @ PopupNotifications.jsm:974:5
 |                    +- PopupNotifications_hide @ PopupNotifications.jsm:713:5
 |                      +- hidePopup @ chrome://global/content/bindings/popup.xml:150:13
 |                        +- <calls calling popupBox.hidePopup that is native>
 |                          +- selfRemovingListener @ RemoteAddonsParent.jsm:610:7
 |                          | +- waitForNotificationClose/</< @ browser_bug553455.js:140:28
 |                          |   +- <closePromise is resolved here>
 |                          |
 |                          +- <test_tabNavigate resumes from await closePromise>
 |                             <test_tabNavigate does await getInstalls()>
 |                             <test_tabNavigate resumes from await getInstalls()>
 |                             <test_tabNavigate fails is(installs.length, 0, ...)>
 |
 +- onLocationChange@resource://gre/modules/AddonManager.jsm:407:5
   +- cancelInstall@resource://gre/modules/AddonManager.jsm:387:7
     +- cancel@resource://gre/modules/addons/XPIInstall.jsm:2657:5
       +- cancel@resource://gre/modules/addons/XPIInstall.jsm:2206:7
         +- cancel@resource://gre/modules/addons/XPIInstall.jsm:1481:7
           +- removeActiveInstall@resource://gre/modules/addons/XPIProvider.jsm:3673:28
            +- <pending install is removed here>

So, instead of waiting for popup hidden event, we should wait for entire onLocationChange listeners finish,
to make sure pending installs are removed.
here's stack trace from PopupBoxObjectBinding::hidePopup to PromiseReactionJob.

>   * frame #0: 0x0000000105135f54 XUL`PromiseReactionJob(JSContext*, unsigned int, JS::Value*) [inlined] PromiseReactionRecord::targetState() at Promise.cpp:393 [opt]
>     frame #1: 0x0000000105135f54 XUL`PromiseReactionJob(JSContext*, unsigned int, JS::Value*) [inlined] PromiseReactionRecord::handler() at Promise.cpp:435 [opt]
>     frame #2: 0x0000000105135f54 XUL`PromiseReactionJob(JSContext*, unsigned int, JS::Value*) [inlined] AsyncFunctionPromiseReactionJob(cx=<unavailable>) at Promise.cpp:1084 [opt]
>     frame #3: 0x0000000105135f54 XUL`PromiseReactionJob(cx=0x0000000110f27000, argc=<unavailable>, vp=0x00007fff5fbf4760) at Promise.cpp:1199 [opt]
>     frame #4: 0x00000001050bc125 XUL`js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [inlined] js::CallJSNative(native=(XUL`PromiseReactionJob(JSContext*, unsigned int, JS::Value*) at Promise.cpp:1169))(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at jscntxtinlines.h:291 [opt]
>     frame #5: 0x00000001050bc0a1 XUL`js::InternalCallOrConstruct(cx=0x0000000110f27000, args=0x00007fff5fbf4718, construct=<unavailable>) at Interpreter.cpp:472 [opt]
>     frame #6: 0x00000001050bc4f9 XUL`js::Call(cx=<unavailable>, fval=<unavailable>, thisv=<unavailable>, args=0x00007fff5fbf4718, rval=<unavailable>) at Interpreter.cpp:540 [opt]
>     frame #7: 0x00000001053eed4a XUL`JS::Call(cx=0x0000000110f27000, thisv=<unavailable>, fval=<unavailable>, args=0x00007fff5fbf47f0, rval=JS::MutableHandleValue @ 0x00007fff5fbf47a0) at jsapi.cpp:3032 [opt]
>     frame #8: 0x0000000102d0bfa2 XUL`mozilla::dom::PromiseJobCallback::Call(this=0x00000001211aed60, cx=0x0000000110f27000, aThisVal=<unavailable>, aRv=0x00007fff5fbf4878) at PromiseBinding.cpp:21 [opt]
>     frame #9: 0x0000000101b63866 XUL`mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) [inlined] mozilla::dom::PromiseJobCallback::Call(this=0x00000001211aed60, aRv=0x0000000100000000, aExecutionReason=<unavailable>, aExceptionHandling=eReportExceptions, aCompartment=<unavailable>) at PromiseBinding.h:89 [opt]
>     frame #10: 0x0000000101b63817 XUL`mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) [inlined] mozilla::dom::PromiseJobCallback::Call(this=0x00000001211aed60) at PromiseBinding.h:104 [opt]
>     frame #11: 0x0000000101b6380d XUL`mozilla::PromiseJobRunnable::Run(this=0x0000000118a64fe0, aAso=0x00007fff5fbf4a70) at CycleCollectedJSContext.cpp:210 [opt]
>     frame #12: 0x0000000101b5a3cb XUL`mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(this=0x00000001005e1000) at CycleCollectedJSContext.cpp:556 [opt]
>     frame #13: 0x00000001033617fc XUL`mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) [inlined] mozilla::CycleCollectedJSContext::LeaveMicroTask(this=<unavailable>) at CycleCollectedJSContext.h:198 [opt]
>     frame #14: 0x00000001033617ec XUL`mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) at CycleCollectedJSContext.h:291 [opt]
>     frame #15: 0x00000001033617e2 XUL`mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) [inlined] mozilla::nsAutoMicroTask::~nsAutoMicroTask() at CycleCollectedJSContext.h:288 [opt]
>     frame #16: 0x00000001033617e2 XUL`mozilla::EventListenerManager::HandleEventSubType(this=<unavailable>, aListener=<unavailable>, aDOMEvent=0x00000001241feb60, aCurrentTarget=0x00000001221bdf70) at EventListenerManager.cpp:1116 [opt]
>     frame #17: 0x00000001033622a9 XUL`mozilla::EventListenerManager::HandleEventInternal(this=<unavailable>, aPresContext=0x00000001211cd000, aEvent=<unavailable>, aDOMEvent=0x00007fff5fbf5020, aCurrentTarget=0x00000001221bdf70, aEventStatus=0x00007fff5fbf5028) at EventListenerManager.cpp:1283 [opt]
>     frame #18: 0x000000010335748c XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5fbf50d0, aVisitor=<unavailable>, aCallback=0x0000000000000000, aCd=0x00007fff5fbf5040) at EventDispatcher.cpp:462 [opt]
>     frame #19: 0x00000001033584b6 XUL`mozilla::EventDispatcher::Dispatch(aTarget=<unavailable>, aPresContext=<unavailable>, aEvent=<unavailable>, aDOMEvent=<unavailable>, aEventStatus=0x00007fff5fbf5218, aCallback=0x0000000000000000, aTargets=<unavailable>) at EventDispatcher.cpp:826 [opt]
>     frame #20: 0x0000000103fcced3 XUL`nsXULPopupManager::HidePopupCallback(this=<unavailable>, aPopup=0x00000001221bdf70, aPopupFrame=0x0000000111e4ad80, aNextPopup=0x0000000000000000, aLastPopup=0x0000000000000000, aPopupType=ePopupTypePanel, aDeselectMenu=<unavailable>) at nsXULPopupManager.cpp:1191 [opt]
>     frame #21: 0x0000000103fcc839 XUL`nsXULPopupManager::FirePopupHidingEvent(this=0x0000000111c503c0, aPopup=0x00000001221bdf70, aNextPopup=0x0000000000000000, aLastPopup=0x0000000000000000, aPresContext=<unavailable>, aPopupType=ePopupTypePanel, aDeselectMenu=<unavailable>, aIsCancel=<unavailable>) at nsXULPopupManager.cpp:1638 [opt]
>     frame #22: 0x0000000103fc92d0 XUL`nsXULPopupManager::HidePopup(this=<unavailable>, aPopup=0x00000001221bdf70, aHideChain=<unavailable>, aDeselectMenu=<unavailable>, aAsynchronous=<unavailable>, aIsCancel=<unavailable>, aLastPopup=<unavailable>) at nsXULPopupManager.cpp:1097 [opt]
>     frame #23: 0x0000000103f980d2 XUL`mozilla::dom::PopupBoxObject::HidePopup(this=0x000000012f6acec0, aCancel=<unavailable>) at PopupBoxObject.cpp:63 [opt]
>     frame #24: 0x0000000102cfeda8 XUL`mozilla::dom::PopupBoxObjectBinding::hidePopup(cx=<unavailable>, obj=<unavailable>, self=0x000000012f6acec0, args=0x00007fff5fbf5530) at PopupBoxObjectBinding.cpp:131 [opt]

so, apparently PopupBoxObjectBinding::hidePopup calls event listeners, and there nsAutoMicroTask handles microtasks.

https://hg.mozilla.org/try/rev/906cc0fd32628e9a639df2497f3906027dd345fc#l8.18
> nsresult
> EventListenerManager::HandleEventSubType(Listener* aListener,
>                                          nsIDOMEvent* aDOMEvent,
>                                          EventTarget* aCurrentTarget)
> {
> ...
>   if (NS_SUCCEEDED(result)) {
>     nsAutoMicroTask mt;
> 
>     // nsIDOMEvent::currentTarget is set in EventDispatcher.
>     if (listenerHolder.HasWebIDLCallback()) {
>       ErrorResult rv;
>       listenerHolder.GetWebIDLCallback()->
>         HandleEvent(aCurrentTarget, *(aDOMEvent->InternalDOMEvent()), rv);
>       result = rv.StealNSResult();
>     } else {
>       result = listenerHolder.GetXPCOMCallback()->HandleEvent(aDOMEvent);
>     }
>   }

actually I'm a bit surprised that microtasks can be executed at this point,
that is, for caller of popupBox.hidePopup (hidePopup in popup.xml), it's just a single function, but it actually can perform microtask checkpoint and can switch to async function.
is it really expected?
Version: 56 Branch → Trunk
There are 2 fixes:
  1. fix waitForSingleNotification to surely wait for single notification
  2. wait for the next tick after closing notification to ensure AddonManager removes pending installs
Attachment #8927235 - Flags: review?(dtownsend)
Attachment #8927235 - Flags: review?(dtownsend) → review+
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae8c03a46ad
Fix events to wait for in browser_bug553455.js. r=mossop
https://hg.mozilla.org/mozilla-central/rev/5ae8c03a46ad
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.