Closed Bug 1094083 Opened 5 years ago Closed 5 years ago

[BrowserAPI] Handle nested-alert gracefully

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla36
blocking-b2g 2.0M+
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.0 --- affected
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: kanru, Assigned: kanru)

References

Details

Attachments

(1 file, 1 obsolete file)

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
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Attached patch WIP (obsolete) — Splinter Review
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)
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 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+
Attached patch patchSplinter Review
Assignee: nobody → kchen
Attachment #8517388 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8517949 - Flags: review?(bugs)
[Blocking Requested - why for this release]: Blocking bug 1082067
Blocks: 1082067
blocking-b2g: --- → 2.1?
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Please request b2g34 approval on this patch when you get a chance.
Flags: needinfo?(kchen)
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?
Attachment #8517949 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
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)
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
Blocks: 1080481, Woodduck
blocking-b2g: 2.1+ → 2.0M+
Hi Kai-Zhen,
Can you help to land this patch on 2.0M? Thanks!
Flags: needinfo?(kli)
Blocks: 1105129
Blocks: Woodduck_P2
Priority: -- → P1
No longer blocks: Woodduck_P2
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.