[BrowserAPI] Handle nested-alert gracefully

RESOLVED FIXED in Firefox 36, Firefox OS v2.0M

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0M+, firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v2.0 affected, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 8517388 [details] [diff] [review]
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."
Attachment #8517388 - Flags: feedback?(bugs)

Comment 2

4 years ago
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

4 years ago
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.
Attachment #8517388 - Flags: feedback?(bugs) → feedback+
(Assignee)

Comment 4

4 years ago
Created attachment 8517949 [details] [diff] [review]
patch
Assignee: nobody → kchen
Attachment #8517388 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8517949 - Flags: review?(bugs)
(Assignee)

Comment 5

4 years ago
[Blocking Requested - why for this release]: Blocking bug 1082067
Blocks: 1082067
blocking-b2g: --- → 2.1?

Updated

4 years ago
Attachment #8517949 - Flags: review?(bugs) → review+
Blocks a blocker so 2.1+
blocking-b2g: 2.1? → 2.1+
https://hg.mozilla.org/mozilla-central/rev/677eee2527e6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Please request b2g34 approval on this patch when you get a chance.
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox34: --- → wontfix
status-firefox35: --- → wontfix
status-firefox36: --- → fixed
Flags: needinfo?(kchen)
(Assignee)

Comment 10

4 years ago
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
Flags: needinfo?(kchen)
Attachment #8517949 - Flags: approval-mozilla-b2g34?

Updated

4 years ago
Attachment #8517949 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+

Comment 11

4 years ago
Unable to verify as it is a back-end issue.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Duplicate of this bug: 1082067
You don't need to set checkin-needed on uplifts. We have bug queries on the B2G Landing page for them.
Keywords: checkin-needed

Updated

4 years ago
Blocks: 1080481, 1054172
blocking-b2g: 2.1+ → 2.0M+
status-b2g-v2.0M: --- → affected

Comment 15

4 years ago
Hi Kai-Zhen,
Can you help to land this patch on 2.0M? Thanks!
Flags: needinfo?(kli)

Updated

4 years ago
status-b2g-v2.0: --- → affected

Updated

4 years ago
Blocks: 1080337

Updated

4 years ago
Blocks: 1105129
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/5a558b26a995
status-b2g-v2.0M: affected → fixed
Flags: needinfo?(kli)

Updated

4 years ago
Blocks: 1107999

Updated

4 years ago
Priority: -- → P1

Updated

4 years ago
No longer blocks: 1107999
You need to log in before you can comment on or make changes to this bug.