Closed Bug 1001851 Opened 6 years ago Closed 6 years ago

Can't attach a downloaded .doc file to bugzilla


(Firefox for Android :: General, defect)

Not set



Firefox 32
Tracking Status
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified
fennec 30+ ---


(Reporter: blassey, Assigned: wesj)




(1 file)

I recieved a .doc file by email, downloaded it to my phone and then tried to attach it to a bug. From the "create attachment" page, click browse, shoows "Documents" and select that .doc. When back to the "create attachment" page the file input still says "No file selected". To confirm this isn't just a rendering glitch, I tried submitting and got "You did not specify a file to attach"

This is a nexus 5 running 4.4.2. This is nightly built from change set b836d89be72b
Does it work for other file types?
I think this happened when we reworked the file picker result handling. None of my phones has this "Documents" thing (is that the KitKat thing?) so I haven't been able to test.
(In reply to Wesley Johnston (:wesj) from comment #2)
> I think this happened when we reworked the file picker result handling. None
> of my phones has this "Documents" thing (is that the KitKat thing?) so I
> haven't been able to test.

Yea, pretty sure the "Documents" app is a kit kat thing.
Assignee: nobody → wjohnston
tracking-fennec: ? → 30+
Probably from bug 971939.
Blocks: 971939
Attached patch PatchSplinter Review
I played with a solution from here [1] that involves getting a persistent url, but wasn't able to get it to work. There are also a lot of people that parse the new uri's for an id, and then requery the system for media [2], but I don't think we know as much as them (i.e. what we're looking at might not be media at all). A quick solution that we can uplift that seems to work well is just to fall back to streaming the uri into a temp file.

Attachment #8421530 - Flags: review?(rnewman)
Two thoughts of how to work around this. First, we could create an subclass of nsIFile that wraps the Android URI that's returned. Second, (but related) have a named pipe that accesses the URI, such that the path can just be passed around.
Duplicate of this bug: 988586
Comment on attachment 8421530 [details] [diff] [review]

Review of attachment 8421530 [details] [diff] [review]:

Heh, always fun to have a patch where every line forces me to go look something up!

This looks fine to me, unless any of the questions below point to a problem or opportunity, and assuming it actually fixes the issue.

Could you also kill the redundant import in while you're here?

::: mobile/android/base/
@@ +141,5 @@
>          }
>          @Override
>          public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) {
>              if (cursor.moveToFirst()) {

The other onLoadFinished implementation in this file does


in an else clause. Is there a reason for not doing so here?

And does the handling change in this loader need to be extended to that, too?

(And these could do with some inversion/early return, but hey, another time.)

@@ +149,5 @@
> +                // Fall back to the normal FileLoader if we didn't find anything.
> +                if (TextUtils.isEmpty(res)) {
> +                    final FragmentActivity fa = (FragmentActivity) GeckoAppShell.getGeckoInterface().getActivity();
> +                    final LoaderManager lm = fa.getSupportLoaderManager();
> +                    lm.initLoader(uri.hashCode(), null, new FileLoaderCallbacks(uri));

Why not reuse the FileLoaderCallbacks instance?
Attachment #8421530 - Flags: review?(rnewman) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment on attachment 8421530 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 971939
User impact if declined: Can't use the new "documents" file picker in kitkat
Testing completed (on m-c, etc.): Been on central for a few weeks and seems ok. We suspected this caused bug 1014161, but it turns out to have been from a different bug. No regressions I know of.
Risk to taking this patch (and alternatives if risky): We could ship with this broken. It only affects kitkat devices AFAIK. Backing out is non-trivial. All alternative patches would probably do something similar. I consider this moderate risk because I don't think file pickers vary across a lot of devices, and its hard to get much testing on them by our nightly users.
String or IDL/UUID changes made by this patch: none.
Attachment #8421530 - Flags: approval-mozilla-beta?
Attachment #8421530 - Flags: approval-mozilla-aurora?
Comment on attachment 8421530 [details] [diff] [review]

Since backing this out is non-trivial and we only have one more beta left in FF30 after today it would be best to get this into a full beta test cycle in 31 instead of pushing it into the end of 30.
Attachment #8421530 - Flags: approval-mozilla-beta?
Attachment #8421530 - Flags: approval-mozilla-beta-
Attachment #8421530 - Flags: approval-mozilla-aurora?
Attachment #8421530 - Flags: approval-mozilla-aurora+
Comment on attachment 8421530 [details] [diff] [review]

I misunderstood a part of the risk assessment re: backing out non-trivial (that referred to the regressing bug, not this patch) and also talked about this in IRC with blassey. This is definitely looking like something we'll actually hear about a fair bit if we do ship it, so changing to approved for Beta uplift and will ask QA to verify before ship that users can:

* Attach something from documents to email
* upload to twitter/facebook from web versions of those services

with this landed.
Attachment #8421530 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Flags: needinfo?(aaron.train)
Verified as fixed in builds:
- 32.0a1 (2014-05-29);
- 31.0a2 (2014-05-29);
Device: Google Nexus 4 (Android 4.4.2).
Flags: needinfo?(aaron.train)
Verified as fixed in build 30 beta 10;
Device: Google Nexus 5 (Android 4.4.2).
You need to log in before you can comment on or make changes to this bug.