InstantAppsHandler initialises ChromePreferenceManager with activity as context. |
||
Issue descriptionIssue observed in instrumentation test: ChromeHomeEnabled preference is not set (when running test with the command line flag to enable it) if its value is not retrieved before calling the instrumentation test's setUp method. I debugged it to realising that the ChromePreferenceManager is sometimes initialised with an activity instead of the application context. E/cr_DGN : PrefMgrInit(org.chromium.chrome.browser.document.ChromeLauncherActivity@42657e30) E/cr_DGN : java.lang.Throwable E/cr_DGN : at org.chromium.chrome.browser.preferences.ChromePreferenceManager.getInstance(ChromePreferenceManager.java:79) E/cr_DGN : at org.chromium.chrome.browser.instantapps.InstantAppsHandler.isChromeDefaultHandler(InstantAppsHandler.java:288) E/cr_DGN : at org.chromium.chrome.browser.instantapps.InstantAppsHandler.handleIncomingIntentInternal(InstantAppsHandler.java:216) E/cr_DGN : at org.chromium.chrome.browser.instantapps.InstantAppsHandler.handleIncomingIntent(InstantAppsHandler.java:175) E/cr_DGN : at org.chromium.chrome.browser.document.ChromeLauncherActivity.doOnCreate(ChromeLauncherActivity.java:202) E/cr_DGN : at org.chromium.chrome.browser.document.ChromeLauncherActivity.onCreate(ChromeLauncherActivity.java:123) E/cr_DGN : at android.app.Activity.performCreate(Activity.java:5231) E/cr_DGN : at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1087) E/cr_DGN : at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2148) E/cr_DGN : at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2233) E/cr_DGN : at android.app.ActivityThread.access$800(ActivityThread.java:135) E/cr_DGN : at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1196) E/cr_DGN : at android.os.Handler.dispatchMessage(Handler.java:102) E/cr_DGN : at android.os.Looper.loop(Looper.java:136) E/cr_DGN : at android.app.ActivityThread.main(ActivityThread.java:5001) E/cr_DGN : at java.lang.reflect.Method.invokeNative(Native Method) E/cr_DGN : at java.lang.reflect.Method.invoke(Method.java:515) E/cr_DGN : at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785) E/cr_DGN : at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601) E/cr_DGN : at dalvik.system.NativeStart.main(Native Method) While when ChromePreferenceManager.getInstance() the first time with the application context obtained from ContextUtils.getApplicationContext or getInstrumentation().getTargetContext(), the preference is properly set and can be retrieved. I can unblock my CL by initialising the preference manager the way that works for my test, but I think the InstantAppsHandler has some hidden bugs related to using the activity instead of the application context in some of the calls.
,
Feb 13 2017
Hmm, looking at ChromePreferenceManager constructor implementation, it does a .getApplicationContext call on the context passed to it. So, it must be holding on to an application context in the manager even if we pass it an activity context.
,
Feb 14 2017
Question: does your test pass if you make a change to InstantAppsHandler.isChromeDefaultHandler() and instead of passing context in, you pass ContextUtils.getApplicationContext() there? Looking at the code I don't see how that can be the case. I would imagine the timing of accessing the manager might be the decisive factor.
,
Feb 14 2017
Okay never mind. I found why my test was failing. The shared preferences are kept across test runs, so depending on the previous test, the preference might be set of not. And since ChromeHome triggers a restart when the preference and the feature flag mismatch, it would crash my test. So I guess this issue could be closed. OTOH, The context is not used anywhere in ChromePreferenceManager, shouldn't it be removed from the constructor?
,
Feb 14 2017
Yeah, I'll make a clean-up change, but just wanted to be clear about what's happening.
,
Feb 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31b20b5b80ab095dafa26b198fd7208d6fef2be5 commit 31b20b5b80ab095dafa26b198fd7208d6fef2be5 Author: mariakhomenko <mariakhomenko@chromium.org> Date: Thu Feb 16 07:18:42 2017 Remove unused context in ChromePreferenceManager. Propagate the removed context change to other classes that were passing context only because they needed to pass it to the ChromePreferenceManager. BUG= 691563 Review-Url: https://codereview.chromium.org/2692203005 Cr-Commit-Position: refs/heads/master@{#450886} [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/CtrSuppression.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/TapSuppressionHeuristics.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninPromoUtil.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/util/FeatureUtilities.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicyTest.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapEventTest.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/MockContextualSearchPolicy.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabFromChromeExternalNavigationTest.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java [modify] https://crrev.com/31b20b5b80ab095dafa26b198fd7208d6fef2be5/chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestCaseBase.java
,
Feb 17 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by mariakho...@chromium.org
, Feb 13 2017