Closed Bug 1414138 Opened 2 years ago Closed 2 years ago

Fix failure of dom/indexedDB/test/test_sandbox.html relying on non-comformant Promise handling

Categories

(Core :: Storage: IndexedDB, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(1 file)

This is a follow-up of the try result in bug 1193394 comment 48:
GECKO(2511) | Assertion failure: !cx->enableAccessValidation || cx->compartment()->isAccessValid(), at /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:394
TEST-UNEXPECTED-TIMEOUT | dom/indexedDB/test/test_sandbox.html | application timed out after 330 seconds with no output
Update my finding so far:
The assertion was hit while enqueuing calling resolve callback at 
http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/dom/indexedDB/test/test_sandbox.html#33
However, similar scenario at
http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/dom/indexedDB/test/test_sandbox.html#23
is totally fine.

Both callbacks are run in the same sandbox without touching its owner window if I replace the resolve/reject callbacks with a empty function.

It seems not related to a problem of a labeled runnable touching a script in different group.

Need investigate more to figure out why we hit this assertion in JS engine.
This is captured if bug 1193394 was fixed.
As explained in bug 1193394 comment 48, to meet the HTML spec in WHATWG, if a promise was settled in a JS callback, the promise callback shall be executed earlier at the return of the outermost JS callback instead of the end of current task at CycleCollectedJSContext::AfterProcessTask:
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/xpcom/threads/nsThread.cpp#1053

In addition, in test_sandbox.html, the last resolved callback in sandbox will try to touch a callback of its container window after onsuccess event handler is returned:
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/dom/indexedDB/test/test_sandbox.html#41

While touching the container window's callback, the ValidatingAccess flag of current labeled SchedulerGroup::Runnable in sandbox is still activated and the SchedulerGroup of the container window is not running, so we hit the assertion mentioned in comment 0.

We didn't see this problem in current m-c because 
1. the promise callback is executed later at CycleCollectedJSContext::AfterProcessTask after SchedulerGroup::Runnable::Run() of current task is returned:
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/xpcom/threads/nsThread.cpp#1053
2. However, the validation flag of the labeled SchedulerGroup::Runnable has been reset earlier at:
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/xpcom/threads/SchedulerGroup.cpp#403

BTW, I am also curious why we need to clear the flag additionally at the end of SchedulerGroup::Runnable::Run() even though it will be reset eventually at the end of nsThread::ProcessNextEvent() when "Maybe<Scheduler::EventLoopActivation> activation" emplaced earlier is auto-destroyed, IIUC.
Attachment #8925433 - Flags: review?(wmccloskey)
Priority: -- → P2
Comment on attachment 8925433 [details] [diff] [review]
(v1) Patch: Keep IndexedDB in sandbox unlabeled.

Review of attachment 8925433 [details] [diff] [review]:
-----------------------------------------------------------------

I guess you should update this comment:
http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/indexedDB/IDBFactory.h#75

I don't think there's a good reason why we clear the validating flag in two places. It seems like a mistake.
Attachment #8925433 - Flags: review?(wmccloskey) → review+
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89994077d440
Keep IndexedDB in sandbox unlabeled. r=billm
https://hg.mozilla.org/mozilla-central/rev/89994077d440
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.