959430 - [B2G][Gallery][Browser Upload]Images cannot be uploaded to browser page if user crops image immediately before sending.

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Gallery, defect, P3)

Description

[Ross Kuhlman ([e-mail])]

Attachment: Log-CroppedImageNotUploaded.txt

Some webpages allow the user to upload images directly from the gallery. If the user is uploading an image, to a webpage and chooses to crop the image before uploading, the image will not be shared properly. 

Repro Steps:
1) Updated Buri to BuildID: 20140113004002
2) Open Browser App.
3) Tap on the URL bar. Type and go to the address: http://robnyman.github.com/Firefox-OS-Boilerplate-App/
4) Select "Pick Image"
5) Select "Gallery"
6) Select a picture.
7) Adjust the picture crop area.
8) Press 'Done' to upload image to site.

Actual:
Cropped image does not appear on webpage.

Expected:
Cropped image is displayed on website.

Environmental Variables:
Device: Buri v1.3 Moz RIL
BuildID: 20140113004002
Gaia: b3fc4f712562ee92b0ed0bd17abc61be9a36a8da
Gecko: 5bb1837de7c0
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Notes:
There is an exception to this issue:
If the user uploads an uncropped image first, afterwards they can crop and upload an image successfully to the same webpage.

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/8909/
See attached: logcats and video.

This issue also occurs in v1.2
Environmental Variables:
Device: Buri v1.2 Moz RIL
BuildID: 20140113004002
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: e672faf1e6a1
Version: 26.0
Firmware Version: v1.2-device.cfg

Comment 1

[Ross Kuhlman ([e-mail])]

Attachment: Log-ImageUploadedCorrectly.txt

Comment 2

[Jason Smith [:jsmith]]

Does this reproduce on 1.1?

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

This bug may be related to OOM issue. I had tested the same STR of comment 0.

1. If we wait for all image thumbnails loaded, the browser app is OOMed.
2. If we choose a small uncropped image first and choose a big image to crop, the browser app is also OOMed.
3. If we choose a small image to crop first, the browser app is not OOMed and image is shown at browser.

I will upload a video to show the case of 3.

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: crop with small image and crop with large image

I use Inari with master version 20140116040206 from pvt build. The first chunk of this video is to crop a small image. The second is to crop a large image. The first one shows correctly. The second one is OOMed.

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Another symptom may be that the browser tab is OOMed when the first opening gallery app with pick activity. It scans all images and create thumbnails for some images not all.

Comment 6

[Josh Schmitt [Joshs]]

Does not reproduce in 1.1, I am able to upload the cropped images from gallery.

Environmental Variables:
Device: Buri 1.1 COM
BuildID: 20140121041200
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Gecko: e7a8e7b9b7d2
Version: 18.0
RIL Version: 01.01.00.019.281
Firmware Version: V1.2-device.cfg

Comment 7

[Jason Smith [:jsmith]]

Image uploading is common place on the web, so having a workflow through the gallery that causes the upload to fail will be problematic in partner apps making use of the pick activity or input=file.

Comment 8

[Preeti Raghunath(:Preeti)]

David,

Please provide your input here for a workaround.

Comment 9

[Hema Koka [:hema]]

johu, could you please work with djf who is taipei this week and see if we are able to address this for 1.3

I am assigning this to you. 

Thanks
Hema

Comment 10

[David Flanagan [:djf]]

If this is an OOM, then it is probably the same thing we fought in bug 935273.

In that bug it was a file upload element in the browser that was causing the image pick and image crop and OOM.  We fixed it by making gallery more efficient at cropping and also by having gecko's filepicker implementation set the "nocrop" flag, so that gallery never even offered to crop the image.

I suspect that is what we need to do here.  But because the web page is initiating its own activity we cannot set the nocrop flag by default.

In bug 935273 it was suggested that we change the image pick activity so that instead of allowing cropping by default, it should not allow cropping by default, and then only allow the crop if the requesting app explicitly requests that choice. We'd have to figure out which apps need cropping and explicitly set the flag for those. I think the contacts app needs it, but I'm not sure if any other do or not.

I thought that was too risky for a koi+ blocker bug, and I still think it would be a change with wide-reaching side effects.  But I don't have other ideas.  Other than waiting for a resolution to bug 854795.

Note that the rationale for making this bug a blocker in comment #7 is not correct. Bug 935273 fixed this bug (by disabling cropping) for the <input type=file> upload case. This bug is only for the case where web content explicitly launches its own activity.  That will be much less common "in the wild".

Given that developers are likely to copy Rob Nyman's code, we could treat this bug as an evangelism thing and ask Rob to modify his demo to set the "nocrop" flag in his pick activity request.

I propose we decide that this bug is not a blocker, ask Rob to modify his demo, and then in 1.4 modify the gallery's pick activity so that cropping is disabled by default and requesting apps have to explicitly request it to get it.

Then

Comment 11

[Jason Smith [:jsmith]]

(In reply to David Flanagan [:djf] from comment #10)
> If this is an OOM, then it is probably the same thing we fought in bug
> 935273.

Agree that this reads off as an OOM.

> 
> In that bug it was a file upload element in the browser that was causing the
> image pick and image crop and OOM.  We fixed it by making gallery more
> efficient at cropping and also by having gecko's filepicker implementation
> set the "nocrop" flag, so that gallery never even offered to crop the image.
> 
> I suspect that is what we need to do here.  But because the web page is
> initiating its own activity we cannot set the nocrop flag by default.

I would argue that that we need to do the opposite here in terms of behavior - apps should be choosing to opt-in into cropping, not choosing to opt out. This is because opting in right now exposes the OOM risk.

It's easier to evangelize an opt-in model with risk associated with it to developers. The opposite negatively affects both our users & apps, as that's defined as the typical use case right now, so existing apps & sites will be negatively affected by the OOM problem.

> 
> Note that the rationale for making this bug a blocker in comment #7 is not
> correct. Bug 935273 fixed this bug (by disabling cropping) for the <input
> type=file> upload case. This bug is only for the case where web content
> explicitly launches its own activity.  That will be much less common "in the
> wild".

It's less common in the web, but that's not true for apps. It's actually more common for apps to decide to use a pick activity to get images. For example, the notes app does this. We have other apps in marketplace that do this as well.

> 
> Given that developers are likely to copy Rob Nyman's code, we could treat
> this bug as an evangelism thing and ask Rob to modify his demo to set the
> "nocrop" flag in his pick activity request.

I don't think this is the right solution. See my discussion above - we should be doing an opt-in model to cropping with risk called out in dev docs, not an opt-out model.

> 
> I propose we decide that this bug is not a blocker, ask Rob to modify his
> demo, and then in 1.4 modify the gallery's pick activity so that cropping is
> disabled by default and requesting apps have to explicitly request it to get
> it.

I have to disagree because of the partner app risk here, the UA sniffing risk, & the evangelism overhead not being worthwhile. If we know that we can't crop on the typical use case, then we shouldn't be exposing it by default. App developers right now operate on the default, not the nocrop flag. We need to fix this bug by switching to an opt-in model for cropping. If we go with a nocrop flag out to developers & later decide to switch to an opt-out model, then we'll run into a UA sniffing problem of the developer having to customize the app on a per version basis.

Note that our app developers expect things to work out of box from 1.1 --> 1.3. If we break that model too much, then we'll negatively break a bunch of our apps on marketplace by caveat without the developer realizing it until they see the negative reviews on marketplace. Our users will suffer as well, as they'll get frustrated with their apps that hit this problem.

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to David Flanagan [:djf] from comment #10)

> I suspect that is what we need to do here.  But because the web page is
> initiating its own activity we cannot set the nocrop flag by default.

It looked like both you and Jason think standardizing opt-in should ideally be the answer here--which I agree with, btw--but this seemed to be the sticking point. 

What's the problem with making this the default for web-initiated (and all the rest) activity? Existing clients dependent on the current behavior?

Comment 13

[Jason Smith [:jsmith]]

I'm waiting for David's input on this based on comment 12.

Comment 14

[David Flanagan [:djf]]

(In reply to Geo Mealer [:geo] from comment #12)
> (In reply to David Flanagan [:djf] from comment #10)
> 
> > I suspect that is what we need to do here.  But because the web page is
> > initiating its own activity we cannot set the nocrop flag by default.
> 
> It looked like both you and Jason think standardizing opt-in should ideally
> be the answer here--which I agree with, btw--but this seemed to be the
> sticking point. 
> 
> What's the problem with making this the default for web-initiated (and all
> the rest) activity? Existing clients dependent on the current behavior?

Geo,

Yes, Jason and I agree that the cropping ought to be opt-in not opt-out. We differ on whether it ought to be fixed in 1.3 or in 1.4. The sentence of mine that you quote doesn't actually make any sense, though, so I'm not sure what I actually meant to say there.

The issue for me is that it seems risky to me to change the default behavior in 1.3. Are you guys willing to carefully QA all apps that do image picking?  Basically, I'm okay with changing it in 1.3 if QA is.

I argued that we don't have to block on this because Rob Nyman's code will be uncommon in the wild and we've already fixed this for the normal input type=file case. Jason countered that we need to fix it for the benefits of apps that do use activities, not for web content.

What Jason does not know, however (because I forgot to mention it) is that when an activity is invoked, the invoking app retains foreground priority so it does not get killed in a low-memory condition. For reasons that I do not understand, however, this does not work for the browser. See the discussion in bug 935273 in comment 64 forward. This browser-can-OOM-when-using-an-activity bug is a consequence of the patch for bug 914412 (see comment 19 in that bug).

So I really do think that this bug only causes an OOM when web content does an image pick and when the user actually crops the image.  This will be very rare and I don't think we need to block on it for 1.3.  But again, I'm okay with doing what Jason and Geo think we should do here.

Comment 15

[Jason Smith [:jsmith]]

Fair enough - I agree with your rationale after finding out that this only applies to web content, not apps. I'll clear the blocking flag then.

Comment 16

[David Flanagan [:djf]]

John: now that this bug is not blocking, do you want to keep it?

Robert: would you be willing to change your boilerplate app so that it passes the 'nocrop' option to the image pick activity to prevent this crash in 1.3?  Otherwise, I fear that many users will copy your code and have similar problems.

Comment 17

[Robert Nyman]

(In reply to David Flanagan [:djf] from comment #16)

> Robert: would you be willing to change your boilerplate app so that it
> passes the 'nocrop' option to the image pick activity to prevent this crash
> in 1.3?  Otherwise, I fear that many users will copy your code and have
> similar problems.

Sure! Just to make sure I get it right, can you show me the exact syntax alternatively send a pull request at https://github.com/robnyman/Firefox-OS-Boilerplate-App?

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to David Flanagan [:djf] from comment #16)
> John: now that this bug is not blocking, do you want to keep it?

No. I will move to NFC first. If I am free, I may be back to this bug. Thanks, David.

Comment 19

[Russ Nicoletti [:russn]]

apps using pick activity with images:

* Contacts (add photo -- cropable)
* Email (attachment -- nocrop)
* Homescreen (Home screen Context menu (?) -- cropable)
* Settings (set wallpaper -- cropable)
* SMS (add image -- cropable)

tests using pick activity with images:

* uitest/js/API/pickphoto.js
* uitest/js/API/picktype.js

Comment 20

[Russ Nicoletti [:russn]]

Attachment: Pull request

Comment 21

[David Flanagan [:djf]]

(In reply to Robert Nyman from comment #17)
> 
> Sure! Just to make sure I get it right, can you show me the exact syntax
> alternatively send a pull request at
> https://github.com/robnyman/Firefox-OS-Boilerplate-App?

I sent a pull request. Thanks!

Comment 22

[David Flanagan [:djf]]

Changing the tracking flags to note that we decided not to attempt fixing this on 1.2 or 1.3

Comment 23

[David Flanagan [:djf]]

Comment on attachment 8371694 [details] [review]
Pull request

Russ,

Overall, this looks really good. r- for some naming and indentation nits, and one minor error (I think) in one of the uitests. Also, I guess I need to ask you to write an integration and/or unit test for this.

Also, there is one spot in gekco (the file picker) where we use nocrop. We'll want to do a followup bug once this lands. Please help me remember to be sure a bug gets filed. And if you want to take that gecko bug, that would be great, if you're set up for building mozilla-central.

Are there any integration tests in any of the apps that exercise cropping?  I'd love to have at least one test that checks the crop enabled and disabled cases.  Have you looked at any of our integration tests? Punam would be a good person to talk to to get started on doing it. And she may be able to point you to other tests that use activities.

I don't have time to test this patch, so please test everything carefully.

Comment 24

[Robert Nyman]

(In reply to David Flanagan [:djf] from comment #21)
> I sent a pull request. Thanks!

Great, just merged it - thanks!

Comment 25

[Russ Nicoletti [:russn]]

David, I've responded to your review comments. One in particular I'd like your opinion on, whether the minimal change is the best option for the unit test changes. Thanks. Pull request: https://github.com/mozilla-b2g/gaia/pull/16056

Comment 26

[Russ Nicoletti [:russn]]

Attachment: Pull request https://github.com/mozilla-b2g/gaia/pull/17039

Comment 27

[Russ Nicoletti [:russn]]

Cancelling review, PR produces integration test error in travis.

Comment 28

[Russ Nicoletti [:russn]]

Attachment: Latest pull request: https://github.com/mozilla-b2g/gaia/pull/17042

Comment 29

[David Flanagan [:djf]]

I'm not sure what happened here. Looks like I never reviewed this and we never changed the crop-by-default behavior of the pick activity. Now that we're a couple of releases further on, I don't feel that we can safely change the behavior now.

But this patch does look like it has a nice integration test in it to verify that the pick activity works between apps. That is something that we ought to salvage if we can.

Russ or Punam: is either of you willing to take this up and get the integration test landed on master?

Comment 30

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #29)
> I'm not sure what happened here. Looks like I never reviewed this and we
> never changed the crop-by-default behavior of the pick activity. Now that
> we're a couple of releases further on, I don't feel that we can safely
> change the behavior now.
> 
> But this patch does look like it has a nice integration test in it to verify
> that the pick activity works between apps. That is something that we ought
> to salvage if we can.
> 
> Russ or Punam: is either of you willing to take this up and get the
> integration test landed on master?

I agree attached patch has good tests that we should use, I will take it up and submit the patch with just integration tests. Thanks

Comment 31

[Bobby Chien]

Per my reproduce on master (2.5) on Flame and Z3C, there is no adjust function in step 7 repro steps (from bug description. 

I would suggest to make this bug as wontfix or worksforme. What do you think?

Comment 32

[Punam Dahiya [:pdahiya]]

As commented in #comment 29 and #comment 30, integration tests for pick activity attached to this bug landed on master as fix of Bug 1080122