Certain WebView singletons can be created twice |
||||
Issue description
Objects in WebViewChromiumAwInit that are initialized in the following way:
public Foo getFoo() {
synchronized (mLock) {
if (mFoo == null) {
ensureChromiumStartedLocked(true);
mFoo = new Foo();
}
}
return mFoo;
}
can cause mFoo to be initalized twice since ensureChromiumStartedLocked(boolean) posts to the UI thread and releases mLock.
So:
1. thread A calls getFoo(), reaches ensureChromiumStartedLocked, posts to the UI thread and releases mLock
2. thread B calls getFoo(), reaches into ensureChromiumStartedLocked
3. the task posted from ensureChromiumStartedLocked from 1 finishes and notifies threads A and B to reacquire mLock.
4. thread A reacquires mLock and sets mFoo = new Foo();
5. thread B reacquires mLock and sets mFoo = new Foo();
5 is unnecessary - it should check whether mFoo has been set and early-out if so:
public Foo getFoo() {
synchronized (mLock) {
if (mFoo == null) {
ensureChromiumStartedLocked(true);
}
// ensureChromiumStartedLocked() posts to the UI thread and releases mLock, so we might have created mFoo when ensureChromiumStartedLocked() returns.
if (mFoo == null) {
mFoo = new Foo();
}
}
return mFoo;
}
,
Feb 28 2018
I might even suggest making ensureChromiumStartedLocked acquire the lock itself and drop the "Locked" suffix from the name if that isn't too annoying a change, because there's not really any hint from the caller's side that it's going to wait on a condition var internally and release the lock. Alternatively: we could use a separate lock to protect the singletons vs the mStarted state of chromium. Probably not worth the effort though, since other than starting chromium all these critical sections *should* be very quick I hope?
,
Feb 28 2018
could check for null mFoo again after ensureChromiumStartedLocked
,
Mar 1 2018
re #2: I'm not sure dropping 'Locked' from ensureChromiumStartedLocked will help much though - sure if we don't hold the lock when calling ensureChromiumStarted then everything is fine, but the caller still won't know that ensureChromiumStarted releases the lock, so there's nothing telling the caller to avoid holding mLock when calling ensureChromiumStarted (and thus making assumptions about mLock being held when ensureChromiumStarted returns). re #3: that's what we suggest in the OP, right? :)
,
Mar 1 2018
Yeah, you're right. If we wanted to make this clearer the best way would be a separate lock for starting chromium but that would instead make a number of the getters more complex, so probably not worth it. Note that there's no need to check mFoo twice in your example. You can just call ensureChromiumStartedLocked unconditionally since it returns immediately if mStarted==true, and then only check mFoo *afterward*.
,
Jun 19 2018
,
Jul 4
,
Jul 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d81a95d87bba1b13fef29061e6ab36fccf6d21cb commit d81a95d87bba1b13fef29061e6ab36fccf6d21cb Author: Tobias Sargeant <tobiasjs@google.com> Date: Tue Jul 10 22:54:25 2018 aw: Avoid WebView(Icon)Database singleton double initialization. In the case where two calls are made to the singleton getter and interleave due to ensureChromiumStartedLocked having released mLock, both calls should not create the singleton instance. Bug: 817444 Change-Id: Ief28fa8818b7d50f921ffd13c0dca0c59ac1fd6b Reviewed-on: https://chromium-review.googlesource.com/1125841 Reviewed-by: Richard Coles <torne@chromium.org> Commit-Queue: Tobias Sargeant <tobiasjs@chromium.org> Cr-Commit-Position: refs/heads/master@{#573965} [modify] https://crrev.com/d81a95d87bba1b13fef29061e6ab36fccf6d21cb/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumAwInit.java
,
Jul 10
|
||||
►
Sign in to add a comment |
||||
Comment 1 by gsennton@chromium.org
, Feb 28 2018