Closed
Bug 1449504
Opened 7 years ago
Closed 7 years ago
Crash/no action when sharing large amount of text from Text Selection Action Bar
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox60 wontfix, firefox61 verified)
VERIFIED
FIXED
Firefox 61
People
(Reporter: sflorean, Assigned: JanH)
Details
Attachments
(2 files)
Environment:
Device: Nexus 5(Android 6.0.1), Huawei MediaPad M2 (Android 5.1.1);
Build: Nightly 61.0a1 (2018-03-27);
Steps to reproduce:
1. Launch Firefox and go to planet.mozilla.org;
2. Long tap on a word and press "select all";
3. Tap on "share" from context menu/action bar.
Expected result:
The text is shared through that option you choose from the list displayed with available apps.
Actual result:
Firefox crashes (for Android 6+).
Action bar is dismiss after tapping on share and text is not shared and is not selected anymore (for Android 5).
Crash: https://crash-stats.mozilla.com/report/index/30c238c0-d1cf-4736-8e1a-22ab20180328
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jh+bugzilla
Hardware: ARM → All
Summary: Crash/no action when sharing from Action Bar → Crash/no action when sharing large amount of text from Text Selection Action Bar
Assignee | ||
Comment 1•7 years ago
|
||
Right now, planet.mozilla.org has ~330k characters. Java uses stores strings as UTF-16 and when launching an ACTION_SEND intent, the EXTRA_TEXT contents are copied into the Intent's ClipData as well, doubling memory requirements once more.
On the other hand according to [1] we can send at most 1 MB via an intent, and in practice even less because that megabyte needs to be shared with any other concurrently running Binder transactions.
Chrome truncates to 100k chars [2].
[1] https://developer.android.com/reference/android/os/TransactionTooLargeException.html
[2] https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/selection/SelectionPopupControllerImpl.java?l=78&rcl=b3e039d5ba5925eefdc46f4c08cea7e02b997aa3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8963717 [details]
Bug 1449504 - Part 1 - Truncate overly long text before sharing via an Intent.
https://reviewboard.mozilla.org/r/232610/#review238306
::: mobile/android/base/java/org/mozilla/gecko/IntentHelper.java:211
(Diff revision 2)
> public static Intent getShareIntent(final Context context,
> - final String targetURI,
> + String targetURI,
> final String mimeType,
> final String title) {
> + if (!TextUtils.isEmpty(targetURI) && targetURI.length() > MAX_INTENT_STRING_DATA_LENGTH) {
> + targetURI = targetURI.substring(0, MAX_INTENT_STRING_DATA_LENGTH) + "…";
Use something like `context.getString(R.string.ellipsis)` because the ellipsis character depends on locale.
::: mobile/android/base/java/org/mozilla/gecko/IntentHelper.java:215
(Diff revision 2)
> + if (!TextUtils.isEmpty(targetURI) && targetURI.length() > MAX_INTENT_STRING_DATA_LENGTH) {
> + targetURI = targetURI.substring(0, MAX_INTENT_STRING_DATA_LENGTH) + "…";
> + }
> +
> Intent shareIntent = getIntentForActionString(Intent.ACTION_SEND);
> shareIntent.putExtra(Intent.EXTRA_TEXT, targetURI);
This patch is fine, though a proper solution is probably to switch to using `EXTRA_STREAM`.
Attachment #8963717 -
Flags: review?(nchen) → review+
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8963717 [details]
Bug 1449504 - Part 1 - Truncate overly long text before sharing via an Intent.
https://reviewboard.mozilla.org/r/232610/#review238306
> This patch is fine, though a proper solution is probably to switch to using `EXTRA_STREAM`.
I can file a follow-up bug for that, but just to be sure that I'm thinking the same thing that you're thinking: You mean something like storing the whole text in a temporary file and then sharing an Uri to that file using EXTRA_STREAM? (And then of course cleaning up that file again some time, maybe on the next app launch - I seem to remember we already have some helper function for cleaning up old unused files)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8963716 [details]
Bug 1449504 - Part 0 - Cleanup unused imports.
https://reviewboard.mozilla.org/r/232608/#review238342
Attachment #8963716 -
Flags: review+
Comment 10•7 years ago
|
||
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/c283a92d8471
Part 0 - Cleanup unused imports. r=JanH
https://hg.mozilla.org/integration/autoland/rev/998fb1b80384
Part 1 - Truncate overly long text before sharing via an Intent. r=jchen
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8963717 [details]
Bug 1449504 - Part 1 - Truncate overly long text before sharing via an Intent.
https://reviewboard.mozilla.org/r/232610/#review238306
> I can file a follow-up bug for that, but just to be sure that I'm thinking the same thing that you're thinking: You mean something like storing the whole text in a temporary file and then sharing an Uri to that file using EXTRA_STREAM? (And then of course cleaning up that file again some time, maybe on the next app launch - I seem to remember we already have some helper function for cleaning up old unused files)
Or I suppose actually implementing a full-blown ContentProvider that would temporarily hold on to the full text and send it out if requested, similar to what was e.g. proposed way back in bug 649371.
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c283a92d8471
https://hg.mozilla.org/mozilla-central/rev/998fb1b80384
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Reporter | ||
Comment 13•7 years ago
|
||
Verified as fixed following the steps from description on latest Nightly build (2018-04-04).
Devices: Nexus 5(Android 6.0.1) and Huawei MediaPad M2 (Android 5.1.1).
Updated•7 years ago
|
tracking-fennec: ? → ---
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•