Closed Bug 1385464 Opened 7 years ago Closed 6 years ago

Build with Android support library 26

Categories

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Platform:
All
Android
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63+ fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 + fixed

People

(Reporter: JanH, Assigned: petru)

References

Details

(Whiteboard: --do_not_change--[priority:high])

Attachments

(1 file, 3 obsolete files)

Bug 1385464 - Start using support library v. 26;
59 bytes, text/x-review-board-request
nalexander
: review+
Details 
      No description provided.
Blocks: 1266683
Blocks: 1385726
No longer blocks: 1266683
Blocks: 1408691
Nick, I think this should be easier now with the Gradle build. It looks to me like newer support libs are only distributed through the Maven repo, and not via the SDK manager as before. If we don't have to care about mozbuild we can just specify that in the gradle files like a civilized person, right? Maybe this is a reason to nuke the mozbuild system?
Flags: needinfo?(nalexander)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #1)
> Nick, I think this should be easier now with the Gradle build. It looks to
> me like newer support libs are only distributed through the Maven repo, and
> not via the SDK manager as before. If we don't have to care about mozbuild
> we can just specify that in the gradle files like a civilized person, right?

Yes.

> Maybe this is a reason to nuke the mozbuild system?

It could be.  If we really need to bump the support libs, then yes -- I think we should finish https://bugzilla.mozilla.org/show_bug.cgi?id=1414415 rather than learning how to fetch an AAR from Google's Maven repo at configure time.
Flags: needinfo?(nalexander)
Blocks: target-android-o
Depends on: 1438716
Blocks: 1459579
Working around the compile errors caused by upgrading the support library version used to 26.1.0 is straightforward enough, but unfortunately after the upgrade I'm encountering RejectedExecutionExceptions during Robocop-4 in automation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c005aa7d93f447014be211fecf1a0c4fdc545bc&selectedJob=177191551

> >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
> java.util.concurrent.RejectedExecutionException: Task android.support.v4.content.ModernAsyncTask$3@420dbc38 rejected from java.util.concurrent.ThreadPoolExecutor@41a952a0[Running, pool size = 128, active threads = 127, queued tasks = 10, completed tasks = 28]
> 	at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(ThreadPoolExecutor.java:1979)
> 	at java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:786)
> 	at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1307)
> 	at android.support.v4.content.ModernAsyncTask.executeOnExecutor(ModernAsyncTask.java:449)
> 	at android.support.v4.content.AsyncTaskLoader.executePendingTask(AsyncTaskLoader.java:225)
> 	at android.support.v4.content.AsyncTaskLoader.onForceLoad(AsyncTaskLoader.java:153)
> 	at android.support.v4.content.Loader.forceLoad(Loader.java:329)
> 	at org.mozilla.gecko.home.BrowserSearch$SuggestionAsyncLoader.onStartLoading(BrowserSearch.java:1037)
> 	at android.support.v4.content.Loader.startLoading(Loader.java:272)
> 	at android.support.v4.app.LoaderManagerImpl$LoaderInfo.start(LoaderManager.java:270)
> 	at android.support.v4.app.LoaderManagerImpl.installLoader(LoaderManager.java:562)
> 	at android.support.v4.app.LoaderManagerImpl.createAndInstallLoader(LoaderManager.java:549)
> 	at android.support.v4.app.LoaderManagerImpl.restartLoader(LoaderManager.java:700)
> 	at org.mozilla.gecko.home.BrowserSearch.filterSuggestions(BrowserSearch.java:707)
> 	at org.mozilla.gecko.home.BrowserSearch.filter(BrowserSearch.java:1002)
> 	at org.mozilla.gecko.BrowserApp.filterEditingMode(BrowserApp.java:2695)
> 	at org.mozilla.gecko.BrowserApp$20.onFilter(BrowserApp.java:1329)
> 	at org.mozilla.gecko.toolbar.ToolbarEditText$TextChangeListener.afterTextChanged(ToolbarEditText.java:569)
> 	at android.widget.TextView.sendAfterTextChanged(TextView.java:7334)
> 	at android.widget.TextView$ChangeWatcher.afterTextChanged(TextView.java:9087)
> 	at android.text.SpannableStringBuilder.sendAfterTextChanged(SpannableStringBuilder.java:970)
> 	at android.text.SpannableStringBuilder.replace(SpannableStringBuilder.java:497)
> 	at android.text.SpannableStringBuilder.replace(SpannableStringBuilder.java:435)
> 	at android.text.SpannableStringBuilder.replace(SpannableStringBuilder.java:30)
> 	at android.text.method.QwertyKeyListener.onKeyDown(QwertyKeyListener.java:223)
> 	at android.text.method.TextKeyListener.onKeyDown(TextKeyListener.java:136)
> 	at android.widget.TextView.doKeyDown(TextView.java:5464)
> 	at android.widget.TextView.onKeyDown(TextView.java:5283)
> 	at android.view.KeyEvent.dispatch(KeyEvent.java:2623)
> 	at android.view.View.dispatchKeyEvent(View.java:7343)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1393)
> 	at com.android.internal.policy.impl.PhoneWindow$DecorView.superDispatchKeyEvent(PhoneWindow.java:1933)
> 	at com.android.internal.policy.impl.PhoneWindow.superDispatchKeyEvent(PhoneWindow.java:1408)
> 	at android.app.Activity.dispatchKeyEvent(Activity.java:2384)
> 	at android.support.v7.app.AppCompatActivity.dispatchKeyEvent(AppCompatActivity.java:534)
> 	at android.support.v7.view.WindowCallbackWrapper.dispatchKeyEvent(WindowCallbackWrapper.java:58)
> 	at android.support.v7.app.AppCompatDelegateImplBase$AppCompatWindowCallbackBase.dispatchKeyEvent(AppCompatDelegateImplBase.java
See Also: → 759338
^^
See above for my current patch, which builds, but suffers from the RejectedExecutionException problem.
Another problem is that at least for local developer builds [1], the total method count is getting dangerously close to the limit.

v23.4.0:
> Total methods in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 63601 (97.05% used)
> Total fields in app-local-withGeckoBinaries-minApi21-photon-debug.apk:  36428 (55.59% used)
> Total classes in app-local-withGeckoBinaries-minApi21-photon-debug.apk:  8055 (12.29% used)

v26.1.0:
> Total methods in app-local-withGeckoBinaries-minApi21-photon-debug.apk: 65131 (99.38% used)
> Total fields in app-local-withGeckoBinaries-minApi21-photon-debug.apk:  41111 (62.73% used)
> Total classes in app-local-withGeckoBinaries-minApi21-photon-debug.apk:  8389 (12.80% used)


[1] Automation seems to have some more breathing space left - picking the currently latest mozilla-central build, I'm seeing "Total methods in app-official-withoutGeckoBinaries-noMinApi-photon-debug.apk: 55006 (83.93% used)". Maybe we're using a more aggressive Proguard configuration there?
Additionally, it seems that the new support library is causing us to leak cursors, because I'm seeing lots of messages like "CursorWrapperInner: Cursor finalized without prior close()" in the logs, which aren't appearing in logs from our current normal automation builds with the previous support library.

Locally, I'm additionally seeing this as well:
> 05-13 05:20:04.898 4768-4777/org.mozilla.fennec_jan E/StrictMode: A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
>     java.lang.Throwable: Explicit termination method 'close' not called
>         at dalvik.system.CloseGuard.open(CloseGuard.java:184)
>         at android.content.ContentResolver$CursorWrapperInner.<init>(ContentResolver.java:1903)
>         at android.content.ContentResolver.query(ContentResolver.java:438)
>         at android.content.ContentResolver.query(ContentResolver.java:357)
>         at org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:684)
>         at org.mozilla.gecko.db.LocalBrowserDB.filter(LocalBrowserDB.java:744)
>         at org.mozilla.gecko.home.SearchLoader$SearchCursorLoader.loadCursor(SearchLoader.java:102)
>         at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:61)
>         at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:42)
>         at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:302)
>         at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:57)
>         at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:45)
>         at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:138)
>         at java.util.concurrent.FutureTask.run(FutureTask.java:234)
>         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1080)
>         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:573)
>         at java.lang.Thread.run(Thread.java:841)
> 05-13 05:20:04.898 4768-4777/org.mozilla.fennec_jan W/CursorWrapperInner: Cursor finalized without prior close()
> 05-13 05:20:05.098 4768-4777/org.mozilla.fennec_jan E/StrictMode: Finalizing a Cursor that has not been deactivated or closed. database = /sdcard/tests/profile/browser.db, table = combined, query = SELECT _id, url, title, bookmark_id, history_id FROM combined WHERE ((url LIKE ? OR title LIKE ?)) GROUP BY url ORDER BY (CASE WHEN bookmark_id > -1 THEN 100 ELSE 0 END) + remoteVisitCount * MAX(1, 100 * 110 / ((1526213958855000 - remoteDateLastVisited) / 86400000000 * (1526213958855000 - remoteDateLastVisited) / 86400000000 + 110)) + (localVisitCount + 2) * (localVisitCount + 2) * MAX(2, 100 * 225 / ((1526213958855000 - localDateLastVisited) / 86400000000 * (1526213958855000 - localDateLastVisited) / 86400000000 + 225)) DESC LIMIT 100
>     android.database.sqlite.DatabaseObjectNotClosedException: Application did not close the cursor or database object that was opened here
>         at android.database.sqlite.SQLiteCursor.<init>(SQLiteCursor.java:98)
>         at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:50)
>         at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1314)
>         at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:400)
>         at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:333)
>         at org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:1458)
>         at android.content.ContentProvider.query(ContentProvider.java:744)
>         at android.content.ContentProvider$Transport.query(ContentProvider.java:199)
>         at android.content.ContentResolver.query(ContentResolver.java:414)
>         at android.content.ContentResolver.query(ContentResolver.java:357)
>         at org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:684)
>         at org.mozilla.gecko.db.LocalBrowserDB.filter(LocalBrowserDB.java:744)
>         at org.mozilla.gecko.home.SearchLoader$SearchCursorLoader.loadCursor(SearchLoader.java:102)
>         at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:61)
>         at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:42)
>         at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:302)
>         at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:57)
>         at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:45)
>         at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:138)
>         at java.util.concurrent.FutureTask.run(FutureTask.java:234)
>         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1080)
>         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:573)
>         at java.lang.Thread.run(Thread.java:841)
The RejectedExecutionProblem and the leaked cursors can be reproduced by running the emulator (I simply used the default Android 4.3 one provided by ./mach android-emulator), clicking the URL bar and then mashing the keyboard/holding down a key. Optionally you can add something like https://hg.mozilla.org/try/rev/97023473565ad87852e264729a090c2b9782c2e2#l2.12 to more easily monitor the current number of threads.

It seems like the problem was already introduced in v24.0.0 of the support library:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=681d35feca2558505e0764ed77bb6bfed195a845

If I didn't overlook anything, the only relevant changes to the Support Library between 23.4.0 and 24.0.0 regarding loaders, tasks, etc. were
https://github.com/aosp-mirror/platform_frameworks_support/commit/0bab42508734d3f701a1fbb1d24c96fabff96163
https://github.com/aosp-mirror/platform_frameworks_support/commit/b3f99a77c492ae252bac148540bcd6e8fa7c18b1
https://github.com/aosp-mirror/platform_frameworks_support/commit/47ab4c24dac8eff1adbe4cc216cbf9c6318993a3

of which I think the last one to be the most likely culprit.
Whiteboard: geckoview → --do_not_change--[priority:high]
Blocks: 1407046
Beside the errors that Jan talked about I see the app crashing when receiving the sign in response with the following stacktrace - https://pastebin.mozilla.org/9086010

I think this is because the current version of play-services and gcm we are using is too low (8.4.0) and incompatible with support library 26 and so it also needs to be updated.
For updating play-services I filed bug 1463376
I have also tried to upgrade the support library used to v. 26 and got the same results as Jan.

- RejectedExecutionException is the biggest problem which started occuring a lot in tests.
I went with Jan's suggestion (thanks!) to force it by spamming the address bar with new text and saw that it is possible to get that crash in the old app also (using previous support library v. 23.4.0) because in the current implementation, for any new input in the address bar a new SearchLoader will be created (an AsyncTaskLoader which will start processing the query on another new thread). Spamming the address bar could create so many AsyncTaskLoaders that the ThreadPoolExecutor is saturated. But indeed it seems that when using support library 26 one can saturate the Executor faster and many Robocop tests are failing because of that [1].
I saw some other complaints about AsyncTaskLoader and RejectedExecutionException [2], [3] but no clear culprit or solution.
Saw that in v. 27 the LoaderManager has been rewritten to better suit ViewModel and LiveData arch components [4] but I've tried using that and saw that same behavior as with v. 26 when spamming the address bar.
The issue seems to stem from repeated calls to Loader().forceLoad() [5] which creates new AsyncTasks (with new threads) and right now to me it seems that getting past it would involve a major refactoring on how we use the loaders for suggestions. 

- The Cursors leaks seems to be tied with the upgrade to support library 26 and I wasn't able to find a clear source for this leaks, like places in which the Cursors should be closed but aren't.
My reasoning for what could be the cause for the leaks would again refer to the multitude of threads possibly created which I think could bring some concurrency issues with reading from the same database.
What I've tried doing to overcome this is to call LoaderManager.destroyLoader() before the existing call to LoaderManager.restartLoader(..) [6] to stop and remove the previous loader. After doing this, all would work as intended, with no crashes.

- Another problem after the update would be (as Jan said) the total number of method references. Building from Android Studio was not possible for me as I would get DexIndexOverflowException. Building with mach was possible though.?

[1] "RejectedExecutionException" and a high number for "current thread count" appear a lot in https://taskcluster-artifacts.net/NoM6AWddRKazXq7IkaMyTQ/0/public/logs/live_backing.log
[2] https://stackoverflow.com/questions/17138095
[3] https://stackoverflow.com/questions/31757881
[4] https://medium.com/google-developers/loaders-in-support-library-27-1-0-b1a1f0fee638
[5] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#1037
[6] https://hg.mozilla.org/try/rev/ae0d59d9f0c3276dcad29899baf6aa613da6ab12#l7.12
I see some tests still failing - https://treeherder.mozilla.org/#/jobs?repo=try&revision=2542cffd07a0fdabc5bb54a9f5fd35083984af01. Please hold off on the review.
Assignee: nobody → petru.lingurar
Attachment #8973769 - Attachment is obsolete: true
Attachment #8980589 - Flags: review?(sdaswani) → review?(michael.l.comella)
Nick, would you be an appropriate reviewer for these patches? If so, please take my r?.

fwiw, it looks like we still might be facing some intermittents based on the comments and a Slack conversation with Petru.
Flags: needinfo?(nalexander)
Comment on attachment 8980587 [details]
Bug 1385464 - Start using support library v. 26;

Hi Jan, can you review this patch and some others I am tagging for r? from you as Mike is busy with pressing deadline work and I don't have r+ access.
Flags: needinfo?(nalexander)
Attachment #8980587 - Flags: review?(sdaswani) → review?(jh+bugzilla)
Attachment #8980588 - Flags: review?(sdaswani) → review?(jh+bugzilla)
Attachment #8980589 - Flags: review?(michael.l.comella) → review?(jh+bugzilla)
Comment on attachment 8980588 [details]
Bug 1385464 - Resolve missing resources and api changes;

https://reviewboard.mozilla.org/r/246740/#review254916

Looks fine, except it seems that something went wrong with the bookmark editing layouts?

::: mobile/android/app/src/main/res/layout/bookmark_add_folder.xml:21
(Diff revision 2)
>          android:id="@+id/toolbar"
>          android:layout_width="match_parent"
>          android:layout_height="?actionBarSize"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
> -        app:navigationIcon="@drawable/abc_ic_clear_mtrl_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"

This doesn't seem right...

::: mobile/android/app/src/main/res/layout/bookmark_add_folder.xml:23
(Diff revision 2)
>          android:layout_height="?actionBarSize"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
> -        app:navigationIcon="@drawable/abc_ic_clear_mtrl_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"
>          app:subtitleTextColor="@android:color/white"
> -        app:title="@string/bookmark_add_folder"
> +        app:title="@string/appbar_scrolling_view_behavior"

... and this even less.

::: mobile/android/app/src/main/res/layout/bookmark_add_folder.xml:47
(Diff revision 2)
>              <EditText
>                  android:id="@+id/edit_folder_name"
>                  android:layout_width="match_parent"
>                  android:layout_height="wrap_content"
>                  android:ellipsize="end"
> -                android:hint="@string/bookmark_edit_name"
> +                android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_add_folder.xml:70
(Diff revision 2)
>                  android:drawableEnd="@drawable/arrow"
>                  android:drawableRight="@drawable/arrow"
>                  android:ellipsize="end"
>                  android:focusable="false"
>                  android:focusableInTouchMode="false"
> -                android:hint="@string/bookmark_parent_folder"
> +                android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:21
(Diff revision 2)
>          android:layout_width="match_parent"
>          android:layout_height="56dp"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
>          android:minHeight="?actionBarSize"
> -        app:navigationIcon="@drawable/abc_ic_clear_mtrl_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:50
(Diff revision 2)
>                      android:id="@+id/edit_bookmark_name"
>                      android:layout_width="match_parent"
>                      android:layout_height="wrap_content"
>                      android:ellipsize="end"
>                      android:gravity="start"
> -                    android:hint="@string/bookmark_edit_name"
> +                    android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:72
(Diff revision 2)
>                      android:id="@+id/edit_bookmark_location"
>                      android:layout_width="match_parent"
>                      android:layout_height="wrap_content"
>                      android:ellipsize="end"
>                      android:gravity="start"
> -                    android:hint="@string/bookmark_edit_location"
> +                    android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:98
(Diff revision 2)
>                      android:drawablePadding="8dp"
>                      android:ellipsize="end"
>                      android:focusable="false"
>                      android:focusableInTouchMode="false"
>                      android:gravity="start"
> -                    android:hint="@string/bookmark_parent_folder"
> +                    android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_edit_with_full_page.xml:119
(Diff revision 2)
>                      android:id="@+id/edit_bookmark_keyword"
>                      android:layout_width="match_parent"
>                      android:layout_height="wrap_content"
>                      android:ellipsize="end"
>                      android:gravity="start"
> -                    android:hint="@string/bookmark_edit_keyword"
> +                    android:hint="@string/appbar_scrolling_view_behavior"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_folder_select.xml:20
(Diff revision 2)
>          android:id="@+id/toolbar"
>          android:layout_width="match_parent"
>          android:layout_height="?actionBarSize"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
> -        app:navigationIcon="@drawable/abc_ic_ab_back_mtrl_am_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"

Ditto.

::: mobile/android/app/src/main/res/layout/bookmark_folder_select.xml:22
(Diff revision 2)
>          android:layout_height="?actionBarSize"
>          android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
>          android:background="@color/text_and_tabs_tray_grey"
> -        app:navigationIcon="@drawable/abc_ic_ab_back_mtrl_am_alpha"
> +        app:navigationIcon="@drawable/ab_add_search_engine"
>          app:subtitleTextColor="@android:color/white"
> -        app:title="@string/bookmark_select_folder" />
> +        app:title="@string/appbar_scrolling_view_behavior" />

Ditto.
Attachment #8980588 - Flags: review?(jh+bugzilla)
Comment on attachment 8980588 [details]
Bug 1385464 - Resolve missing resources and api changes;

https://reviewboard.mozilla.org/r/246740/#review254920

Also note that lint appears to throw up some additional errors. Two of these are because of the bookmark edit layout issues, but I think the others here need to be addressed, too:
https://taskcluster-artifacts.net/Ba9FGDtARt-rLSSVfFMQtA/0/public/android/lint/lint-results-officialWithoutGeckoBinariesNoMinApiPhotonDebug.html#RestrictedApi
Thanks Jan! Will look into them.
Attachment #8980589 - Flags: review?(jh+bugzilla) → review?(gkruglov)
Comment on attachment 8980589 [details]
Bug 1385464 - Resolve obscure leaks and crashes after the upgrade;

https://reviewboard.mozilla.org/r/246742/#review255186

I'm not entirely happy about simply disabling that code - even if we weren't actively testing it, getting it exercised during the other tests was still useful as a very basic check. Short of a bigger rewrite of the suggestion system there might be no better alternative, though.
I also hope that this problem is indeed specific to the emulator and not actually relevant during real-life usage where we've hopefully got a better ratio of device speed to typing speed.
Still, I'd like someone else to take a second look at this as well if possible.
Comment on attachment 8980588 [details]
Bug 1385464 - Resolve missing resources and api changes;

https://reviewboard.mozilla.org/r/246740/#review256636

Looks fine, thanks.
Attachment #8980588 - Flags: review?(jh+bugzilla) → review+
(In reply to Petru-Mugurel Lingurar[:petru] from comment #11)
> For updating play-services I filed bug 1463376

Please don't forget to set the proper dependencies, otherwise there's a risk of overlooking this.
Depends on: 1463376
Comment on attachment 8980587 [details]
Bug 1385464 - Start using support library v. 26;

https://reviewboard.mozilla.org/r/246738/#review256890

Who ever okays the cursor loader changes can give the final r+ here as well. If Grisha isn't around, maybe ask nalexander or mcomella?

The one recommendation I'd make is that after all patches have been successfully reviewed, you should squash them together before landing (but please preserve the full commit messages for the individual parts), since you'd end up with broken commits that cannot be built otherwise.
Attachment #8980587 - Flags: review?(jh+bugzilla)
Attachment #8980589 - Flags: review?(gkruglov) → review?(nalexander)
Comment on attachment 8980587 [details]
Bug 1385464 - Start using support library v. 26;

https://reviewboard.mozilla.org/r/246738/#review257056

This impacts GeckoView (and geckoview_example), but I think this is fine.  I'll verify with the GV team.

::: build.gradle:54
(Diff revision 3)
>              url "file://${gradle.mozconfig.topsrcdir}/mobile/android/gradle/m2repo"
>          }
>      }
>  
>      ext.kotlin_version = '1.2.41'
> -    ext.support_library_version = '23.4.0'
> +    ext.support_library_version = '26.1.0'

It's amazing that this is all that is required to bump the support library version these days!
Attachment #8980587 - Flags: review+
Comment on attachment 8980588 [details]
Bug 1385464 - Resolve missing resources and api changes;

https://reviewboard.mozilla.org/r/246740/#review257058

This all looks fine.
Attachment #8980588 - Flags: review+
Depends on: 1468487
Comment on attachment 8980589 [details]
Bug 1385464 - Resolve obscure leaks and crashes after the upgrade;

https://reviewboard.mozilla.org/r/246742/#review257060

I hate changing tests in automation, but fine -- whatever gets us through the day.

Thanks for pushing this forward, Petru and JanH!

::: mobile/android/base/java/org/mozilla/gecko/home/SearchLoader.java:75
(Diff revision 3)
>      }
>  
>      public static void restart(LoaderManager manager, int loaderId,
>                                 LoaderCallbacks<Cursor> callbacks, String searchTerm,
>                                 EnumSet<FilterFlags> flags) {
> +        manager.destroyLoader(loaderId);

Oh, this is awful, but based on my reading, this whole API is deprecated and other people have had to do similar things to make loaders work at all :/
Attachment #8980589 - Flags: review?(nalexander) → review+
I see lint failures in https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7bcf91575c831e905930a74a460b093ec60423a&selectedJob=181632112 -- those need to be fixed before landing (but no re-review needed).

dbolter: this patch will bump the support library version for GeckoView built in automation.  I think that's the right thing to do but could you stamp yourself or redirect to somebody on your team who can OK this bump?
Flags: needinfo?(petru.lingurar)
Flags: needinfo?(dbolter)
(In reply to Nick Alexander :nalexander from comment #35)
> I see lint failures in
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b7bcf91575c831e905930a74a460b093ec60423a&selectedJob=1
> 81632112 -- those need to be fixed before landing (but no re-review needed).
> 
> dbolter: this patch will bump the support library version for GeckoView
> built in automation.  I think that's the right thing to do but could you
> stamp yourself or redirect to somebody on your team who can OK this bump?

This Lint issues were reported by :JanH in comment 23 and resolved in latest version of the patches.

I triggered a try build with the same syntax to confirm the issues gone and now I see some strange L10n tests errors - https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf8baddf524530058f342735ca795ff64c7c3990 
Don't know what's up with these and I don't think they were caused by this patch..
Flags: needinfo?(petru.lingurar)
Attachment #8980588 - Attachment is obsolete: true
Attachment #8980589 - Attachment is obsolete: true
Squashed the 3 patches together as Jan suggested, waiting for the 63 nightly and the OK from dbolter to merge.
(In reply to Petru-Mugurel Lingurar[:petru] from comment #38)
> Squashed the 3 patches together as Jan suggested, waiting for the 63 nightly
> and the OK from dbolter to merge.

Looks good.  The l10n errors are definitely TC scheduling errors; let's not worry about them.
Over to James. (see comment 35)
Flags: needinfo?(dbolter) → needinfo?(snorp)
Status: NEW → ASSIGNED
Blocks: 1463376
No longer depends on: 1463376
We're ok with SDK 26 for GeckoView, assuming tests pass.
Flags: needinfo?(snorp)
David: Can this land in nightly 63 now?
Flags: needinfo?(dbolter)
(In reply to Marcia Knous [:marcia - needinfo? me] from comment #42)
> David: Can this land in nightly 63 now?

Or as jcristau pointed out, will Bug 1463376#c0 be an issue?
(In reply to Marcia Knous [:marcia - needinfo? me] from comment #43)
> (In reply to Marcia Knous [:marcia - needinfo? me] from comment #42)
> > David: Can this land in nightly 63 now?
> 
> Or as jcristau pointed out, will Bug 1463376#c0 be an issue?

If those two don't land together crashes are guaranteed.
Sounds like Petru-Mugurel answered. Adding some NIs for awareness. Someone should figure out how to coordinate these related landings (I can't take this on right now).
Flags: needinfo?(nalexander)
Flags: needinfo?(dbolter)
Flags: needinfo?(andrei.a.lazar)
Blocks: 1450447
Blocks: 1473518
Small rebase was needed to be able to land this on top bug 1463376.
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b1f1b52866
Start using support library v. 26; r=nalexander
https://hg.mozilla.org/mozilla-central/rev/c2b1f1b52866
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #45)
> Sounds like Petru-Mugurel answered. Adding some NIs for awareness. Someone
> should figure out how to coordinate these related landings (I can't take
> this on right now).

This has landed, so I'm clearing NI.
Flags: needinfo?(nalexander)
Flags: needinfo?(andrei.a.lazar)
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 63 → mozilla63
See Also: → 1584169
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: