1094083 - [BrowserAPI] Handle nested-alert gracefully

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

In bug 1082067, the following script

navigator.mozApps.getSelf().onsuccess = () => {
  console.log('1st alert begin');
  alert(1);
  console.log('1st alert end');
};
navigator.mozApps.getSelf().onsuccess = () => {
  console.log('2nd alert begin');
  alert(2);
  console.log('2nd alert end');
};

makes the child frame stuck in the nested event loop.



I/Gecko   (19119): BrowserElementChild - _waitForResult([object XrayWrapper [object Window]])
I/Gecko   (19119): BrowserElementChild - Entering modal state (outerWindowID=1, innerWindowID=4)
I/Gecko   (19119): BrowserElementChild - Nested event loop - begin
I/Gecko   ( 8526): BrowserElementParent.jsm - handleShowPrompt {"promptType":"alert","title":"The page at app://clock.gaiamobile.org says:","message":"1","windowID":{"outer":1,"inner":4},"msg_name":"showmodalprompt"}
I/Gecko   ( 8526): BrowserElementParent.jsm - Event will have detail: {"promptType":"alert","title":"The page at app://clock.gaiamobile.org says:","message":"1","msg_name":"showmodalprompt"}
I/Gecko   (19119): BrowserElementChild - _waitForResult([object XrayWrapper [object Window]])
I/Gecko   (19119): BrowserElementChild - Entering modal state (outerWindowID=1, innerWindowID=4)
I/Gecko   (19119): BrowserElementChild - Nested event loop - begin
E/GeckoConsole(19119): Content JS LOG at Scratchpad/1:11 in anonymous: 1st alert begin
E/GeckoConsole(19119): Content JS LOG at Scratchpad/1:16 in anonymous: 2nd alert begin
I/Gecko   ( 8526): BrowserElementParent.jsm - handleShowPrompt {"promptType":"alert","title":"The page at app://clock.gaiamobile.org says:","message":"2","windowID":{"outer":1,"inner":4},"msg_name":"showmodalprompt"}
I/Gecko   ( 8526): BrowserElementParent.jsm - Event will have detail: {"promptType":"alert","title":"The page at app://clock.gaiamobile.org says:","message":"2","msg_name":"showmodalprompt"}
D/wpa_supplicant(16524): RX ctrl_iface - hexdump(len=11): 53 49 47 4e 41 4c 5f 50 4f 4c 4c
D/wpa_supplicant(16524): wlan0: Control interface command 'SIGNAL_POLL'
D/wpa_supplicant(16524): nl80211: survey data missing!
I/Gecko   (19119): BrowserElementChild - recvStopWaiting(outer=1, inner=4, returnValue=undefined)
I/Gecko   (19119): BrowserElementChild - recvStopWaiting [object XrayWrapper [object Window]]
I/Gecko   (19119): BrowserElementChild - Nested event loop - finish
I/Gecko   (19119): BrowserElementChild - Leaving modal state (outerID=1, innerID=4)
E/GeckoConsole(19119): Content JS LOG at Scratchpad/1:18 in anonymous: 2nd alert end
I/Gecko   (19119): BrowserElementChild - recvStopWaiting(outer=1, inner=4, returnValue=undefined)
I/Gecko   (19119): BrowserElementChild - recvStopWaiting: No record of outer window ID 1
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP

This patch should fix this. I'm writing the test case but I'm wondering if this behavior is sane. That is, is nest-alert normal?

Run following code on desktop shows that we could have nested alert on desktop

(function(){
  var xhr = new XMLHttpRequest();
  xhr.addEventListener("load", ()=>alert("xhr"));
  xhr.open('GET', window.location);
  xhr.send();
  alert("alert");
  console.log("after alert");
})();

that also means we could run the listener code out of order, i.e. before "after alert". The spec says "Logic that depends on tasks or microtasks, such as media elements loading their media data, are stalled when these methods [alert, confirm, prompt] are invoked."

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Nested alerts aren't something which should happen, since alert() should be sync. But in Gecko
alert() (at least in single process Gecko) must spin the event loop in order to accept user input.

Note, as far as I know, our Promises don't use microtasks yet, but task, and task are the thing in the main event loop.

(MutationObserver which relies on microtasks, tries to deal with common sync operation and not run callbacks during them.)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8517388 [details] [diff] [review]
WIP

Ah, yes, we don't want this._windowIDDict[outerID] to be gone until the last modal prompt is closed.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: patch

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

[Blocking Requested - why for this release]: Blocking bug 1082067

Comment 6

[Andrew Overholt [:overholt]]

Blocks a blocker so 2.1+

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/677eee2527e6

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/677eee2527e6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request b2g34 approval on this patch when you get a chance.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8517949 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Using alert() in listeners or callbacks might break setTimeout() and others things that don't run in nested event loop. It affects apps on Firefox OS more because alert()s are more common.
Testing completed: landed on mozilla-central with test case
Risk to taking this patch (and alternatives if risky): little, the change is only 3 lines so we could easily backout if something went wrong.
String or UUID changes made by this patch: N/A

Comment 11

[sarsenyev]

Unable to verify as it is a back-end issue.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

You don't need to set checkin-needed on uplifts. We have bug queries on the B2G Landing page for them.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/1dc8f1fd29de

Comment 15

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Can you help to land this patch on 2.0M? Thanks!

Comment 16

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/5a558b26a995