Closed
Bug 1214516
Opened 9 years ago
Closed 9 years ago
Create IDBFactory background actor directly if PBackgroundChild is existed
Categories
(Core :: Storage: IndexedDB, defect, P2)
Core
Storage: IndexedDB
Tracking
()
People
(Reporter: ting, Assigned: ting)
References
Details
Attachments
(1 file, 1 obsolete file)
2.56 KB,
patch
|
Details | Diff | Splinter Review |
Observed this from bug 1210860, which the indexed DB openning request is not initiated right away even PBackgroundChild is existed. It's bceause GetOrCreateForCurrentThread() is used, we can use GetForCurrentThread() to check beforehand.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8673551 -
Flags: review?(khuey)
There was a reason for this ... maybe bent remembers? It may have just been to make the code behave identically regardless of whether or not PBackground was already active.
Flags: needinfo?(bent.mozilla)
Comment 3•9 years ago
|
||
Yeah, I remember one version of IDB on PBackground had the GetForCurrentThread() call, but it was removed before landing.
It's because you can get actor order wrong. Say the first IDB user doesn't have a PBackground yet. It will get queued, and PBackground setup runnables will get added into the event loop. At some point PBackground setup completes (so that GetForCurrentThread() will return a non-null value) and the original IDB runnable will get posted to the event loop to start using IDB-on-PBackground. If another runnable that uses IDB stuff was already in the queue then it could run before the original.
Flags: needinfo?(bent.mozilla)
(In reply to Ben Turner (not reading bugmail, use the needinfo flag!) from comment #4) > If another runnable that uses IDB stuff was already in > the queue then it could run before the original. ... if we switched to using GetForCurrentThread(). We use GetOrCreateForCurrentThread() so that the second will get bumped to the end of the event loop and order will be maintained.
Assignee | ||
Comment 6•9 years ago
|
||
Great, then the patch should work. The predondition is still: if (!mBackgroundActor && mPendingRequests.IsEmpty()) { which will do GetForCurrentThread() only when there's no pending open/delete request, also mBackgroundActor is now only set by the callback. So the ordering issue Ben mentioned won't happen. BTW, it's not switch to GetForCurrentThread(), but first check by GetForCurrentThread(), use it if it exists otherwise go original path with GetOrCreateForCurrentThread().
Sorry, that condition is per-IDBFactory, not per-PBackgroundChild. The ordering problem is between IDB users in different windows.
Assignee | ||
Comment 8•9 years ago
|
||
So the problem exists if there's an user does not have a PBackgroudChild, but don't [1] [2] guarantee this won't happen? [1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#788 [2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#1839
The problem doesn't exist on workers because we block each worker until PBackground is established: http://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2978 The main thread is not so lucky, in the parent process or the child process. There's always a window of time where GetForCurrentThread() will return null.
Comment on attachment 8673551 [details] [diff] [review] bug-1214516.v1.patch Review of attachment 8673551 [details] [diff] [review]: ----------------------------------------------------------------- I'm convinced that Ting-Yu is correct and that bent is wrong. When we create a PBackgroundChild actor the queued callbacks are all executed immediately before returning to the event loop.[0] Thus it's not possible for an event to insert itself "between" the creation of the PBackground and the execution of all queued callbacks. The BackgroundActorCreated callbacks will be executed in the order the IDB calls were made. In other words, making BackgroundChild::GetForCurrentThread return non-null and executing all the queued callbacks is an atomic operation. r=me [0] http://hg.mozilla.org/mozilla-central/annotate/d1a89632277f/ipc/glue/BackgroundImpl.cpp#l1962 and http://hg.mozilla.org/mozilla-central/annotate/d1a89632277f/ipc/glue/BackgroundImpl.cpp#l1896 ::: dom/indexedDB/IDBFactory.cpp @@ +713,5 @@ > newIDBThreadLocal = idbThreadLocal = new ThreadLocal(id); > } > > + PBackgroundChild* actor = BackgroundChild::GetForCurrentThread(); > + if (actor) { nit: I prefer the following style if (PBackgroundChild* actor = ...) {
Attachment #8673551 -
Flags: review?(khuey) → review+
Assignee: nobody → janus926
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•9 years ago
|
||
Rebased, addressed nit, added r=khuey. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc44db95b0b0 Thank you, Kyle and Ben.
Attachment #8673551 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
There's a test case failed on Try: 106 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_aboutHome.js | Test timed out - Running the test locally is passed.
Comment 13•9 years ago
|
||
(In reply to Ting-Yu Chou [:ting] from comment #12) > There's a test case failed on Try: > > 106 INFO TEST-UNEXPECTED-FAIL | > browser/base/content/test/general/browser_aboutHome.js | Test timed out - > > Running the test locally is passed. I observed the same problem with my patch for bug 961049. The test is obviously wrong. Sometimes the default search engine is set too late and the test then times out. Try to extract browser_aboutHome.js changes from my patch and push to try again. I hope this saves you a lot of time :)
Assignee | ||
Comment 14•9 years ago
|
||
Thanks Jan, it gets passed with your changes [1], but I'm curious why a performance improvement could make a test case permanent timed out. I'll still take a look. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1194ad5d5440
Comment 15•9 years ago
|
||
Yeah it's strange, but it's actually a race. You removed a main thread hop in C++ which blocked code in the test from progressing faster.
Assignee | ||
Comment 16•9 years ago
|
||
Jan's right, it's a race. The test case triggers ContentSearchUIController._onInput() with "x" but _getSuggestions() does nothing since |defaultEngine| is still undefined, so it timed out. The |defaultEngine| is expected to be set from the result of asynchronous message "GetState", which is sent when ContentSearchUIController is instantiated. Jan's patch works since it reset the engine and wait for message "CurrentState" to set |defaultEngine| before starting the test.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > I'm convinced that Ting-Yu is correct and that bent is wrong. Happy to be corrected!
Assignee | ||
Comment 18•9 years ago
|
||
I'll just wait for the test fixes in bug 961049 landing.
Updated•9 years ago
|
Assignee | ||
Comment 19•9 years ago
|
||
Bug 961049 just landed, retry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c56ee89e691
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e96f99116f62
Keywords: checkin-needed
Comment 22•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e96f99116f62
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•