Build with Android support library 26

RESOLVED FIXED in Firefox 63

Status

()

RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: JanH, Assigned: petru)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

Trunk
Firefox 63
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63+ fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

a year ago
Blocks: 1266683
(Reporter)

Updated

a year ago
Blocks: 1385726
(Reporter)

Updated

a year ago
No longer blocks: 1266683
(Reporter)

Updated

a year ago
Blocks: 1408691
Whiteboard: geckoview
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)
(Reporter)

Updated

7 months ago
Blocks: 1450450
(Reporter)

Updated

7 months ago
Depends on: 1438716
(Reporter)

Updated

7 months ago
Blocks: 1459579
(Reporter)

Comment 3

7 months ago
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
(Reporter)

Updated

7 months ago
See Also: → bug 759338
Comment hidden (mozreview-request)
(Reporter)

Comment 5

7 months ago
^^
See above for my current patch, which builds, but suffers from the RejectedExecutionException problem.
(Reporter)

Comment 6

7 months ago
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?
(Reporter)

Comment 7

6 months ago
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)
(Reporter)

Comment 8

6 months ago
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.

Updated

6 months ago
Whiteboard: geckoview → --do_not_change--[priority:high]
(Reporter)

Updated

6 months ago
Duplicate of this bug: 1333704
(Assignee)

Updated

6 months ago
Blocks: 1407046
(Assignee)

Comment 10

6 months ago
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.
(Assignee)

Comment 11

6 months ago
For updating play-services I filed bug 1463376
(Assignee)

Comment 12

6 months ago
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/﷒0﷓
[6] https://hg.mozilla.org/try/rev/ae0d59d9f0c3276dcad29899baf6aa613da6ab12#l7.12
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

6 months ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

6 months ago
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)
(Reporter)

Comment 22

6 months ago
mozreview-review
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)
(Reporter)

Comment 23

6 months ago
mozreview-review
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
(Assignee)

Comment 24

6 months ago
Thanks Jan! Will look into them.
(Reporter)

Updated

6 months ago
Attachment #8980589 - Flags: review?(jh+bugzilla) → review?(gkruglov)
(Reporter)

Comment 25

6 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 29

6 months ago
mozreview-review
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+
(Reporter)

Comment 30

6 months ago
(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
(Reporter)

Comment 31

5 months ago
mozreview-review
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)
(Assignee)

Updated

5 months ago
Attachment #8980589 - Flags: review?(gkruglov) → review?(nalexander)

Comment 32

5 months ago
mozreview-review
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 33

5 months ago
mozreview-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+
(Reporter)

Updated

5 months ago
Depends on: 1468487

Comment 34

5 months ago
mozreview-review
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)
(Assignee)

Comment 36

5 months ago
(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)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8980588 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8980589 - Attachment is obsolete: true
(Assignee)

Comment 38

5 months ago
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)

Updated

5 months ago
Status: NEW → ASSIGNED

Updated

5 months ago
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?
(Assignee)

Comment 44

5 months ago
(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)

Updated

5 months ago
Blocks: 1450447

Updated

5 months ago
tracking-firefox63: --- → +

Updated

5 months ago
Blocks: 1473518
Comment hidden (mozreview-request)
(Assignee)

Comment 47

5 months ago
Small rebase was needed to be able to land this on top bug 1463376.

Updated

4 months ago
Keywords: checkin-needed
Comment hidden (mozreview-request)

Updated

4 months ago
status-firefox63: --- → affected
Keywords: checkin-needed

Comment 49

4 months ago
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b1f1b52866
Start using support library v. 26; r=nalexander

Comment 50

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2b1f1b52866
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months 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)

Updated

4 months ago
Flags: needinfo?(andrei.a.lazar)
You need to log in before you can comment on or make changes to this bug.