Commit 8463895c authored by sunangel@chromium.org's avatar sunangel@chromium.org

Ensure Java DistilledPagePrefs observers can only be added once.

The Java DistilledPagePrefs observers are wrapped in a
native observer, which is only deleted when the observer is
removed again. Since the reference to this native observer
wrapper is kept in a Java map with the real Java observer as
the key, the second time addObserver(foo) is called with the
same observer, the first reference is lost, and the native
object is leaked. 

For example, the following would leak a native object:
addObserver(foo);
addObserver(foo);
removeObserver(foo);
removeObserver(foo);

It is already safe to remove an observer if it is not
currently an observer, and this CL ensures that an observer
can only be added once, so the second calls to
addObserver(foo) and removeObserver(foo) will be no-ops.

addObserver and removeObserver will return a boolean based
on whether they have successfully completed the operation.

BUG=383630

Review URL: https://codereview.chromium.org/426153006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287911 0039d316-1c4b-4281-b951-d872f2087c98
parent 021265ed
......@@ -92,6 +92,22 @@ public class DistilledPagePrefsTest extends ChromeShellTestBase {
mDistilledPagePrefs.removeObserver(testObserverTwo);
}
@SmallTest
@Feature({"DomDistiller"})
public void testRepeatedAddAndDeleteObserver() throws InterruptedException {
TestingObserver test = new TestingObserver();
// Should successfully add the observer the first time.
assertTrue(mDistilledPagePrefs.addObserver(test));
// Observer cannot be added again, should return false.
assertFalse(mDistilledPagePrefs.addObserver(test));
// Delete the observer the first time.
assertTrue(mDistilledPagePrefs.removeObserver(test));
// Observer cannot be deleted again, should return false.
assertFalse(mDistilledPagePrefs.removeObserver(test));
}
private static class TestingObserver implements DistilledPagePrefs.Observer {
private Theme mTheme;
......
......@@ -65,19 +65,29 @@ public final class DistilledPagePrefs {
mObserverMap = new HashMap<Observer, DistilledPagePrefsObserverWrapper>();
}
public void addObserver(Observer obs) {
/*
* Adds the observer to listen to changes in DistilledPagePrefs.
* @return whether the observerMap was changed as a result of the call.
*/
public boolean addObserver(Observer obs) {
if (mObserverMap.containsKey(obs)) return false;
DistilledPagePrefsObserverWrapper wrappedObserver =
new DistilledPagePrefsObserverWrapper(obs);
nativeAddObserver(mDistilledPagePrefsAndroid, wrappedObserver.getNativePtr());
mObserverMap.put(obs, wrappedObserver);
return true;
}
public void removeObserver(Observer obs) {
/*
* Removes the observer and unregisters it from DistilledPagePrefs changes.
* @return whether the observer was removed as a result of the call.
*/
public boolean removeObserver(Observer obs) {
DistilledPagePrefsObserverWrapper wrappedObserver = mObserverMap.remove(obs);
if (wrappedObserver != null) {
nativeRemoveObserver(mDistilledPagePrefsAndroid, wrappedObserver.getNativePtr());
wrappedObserver.destroy();
}
if (wrappedObserver == null) return false;
nativeRemoveObserver(mDistilledPagePrefsAndroid, wrappedObserver.getNativePtr());
wrappedObserver.destroy();
return true;
}
public void setTheme(Theme theme) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment