996141 - Unable to upload an image to web page using the "Pick Image" button on the Boilerplate app - http://robnyman.github.com/Firefox-OS-Boilerplate-App/

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Joshua Mitchell (Inactive)]

Attachment: logcat.txt

Description:
Selecting the Pick Image button will take you to your gallery where you can then select an image to upload. Selecting the image and then selecting done will return you to the webpage but the photo never uploads to it. This was attempted with a variety of photos ranging in size, including several taken with the Tarako device itself.

Repro Steps:
1) Update a Tarako to BuildID: 20140414060052
2) Launch the Browser
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) Select Done
Actual:
User is returned to webpage, photo is not uploaded / shown on webpage
Expected:
User will be returned to webpage with photo uploaded

1.3T Environmental Variables:
Device: Tarako 1.3T MOZ
BuildID: 20140414060052
Gaia: bbba09c732c35d9434ecb04d5e2e41d05511f4d9
Gecko:
Version: 28.1
Firmware Version: SP6821

Repro frequency:100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/8909/
See attached: Logcat.txt, Firewatch_Boilerplate.txt

this does NOT repro on the 1.3 Buri
1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140409004002
Gaia: 100a159af11281d965e9d70d0f3d3111a38775ee
Gecko: ea2a473b122f
Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 1

[Joshua Mitchell (Inactive)]

Attachment: firewatch_Boilerplate.txt

Comment 2

[Jason Smith [:jsmith]]

The test case looks like the following:

    var pickImage = document.querySelector("#pick-image");
    if (pickImage) {
        pickImage.onclick = function () {
            var pick = new MozActivity({
                name: "pick",
                data: {
                    type: ["image/png", "image/jpg", "image/jpeg"],
                    // In FxOS 1.3 and before the user is allowed to crop the
                    // image by default, but this can cause out-of-memory issues
                    // so we explicitly disable it.
                    nocrop: true // don't allow the user to crop the image
                }
            });

            pick.onsuccess = function () {
                var img = document.createElement("img");
                img.src = window.URL.createObjectURL(this.result.blob);
                var imagePresenter = document.querySelector("#image-presenter");
                imagePresenter.appendChild(img);
                imagePresenter.style.display = "block";
            };

            pick.onerror = function () {
                console.log("Can't view the image");
            };
        };
    }

This is the basic test case for picking an image without cropping the image, which should work. If this doesn't work, then you won't be able to upload images to web content, which will break a core piece of the web (e.g. uploading images to Facebook, uploading images to Twitter, etc). As such, this is a blocker.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ for broken use case

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Confirmed with Marvin offline, we decided to turn off the the image cropper in the gallery picker for tarako. John will help to make a switch for build time customization.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jason Smith [:jsmith] from comment #2)
>             var pick = new MozActivity({
>                 name: "pick",
>                 data: {
>                     type: ["image/png", "image/jpg", "image/jpeg"],
>                     // In FxOS 1.3 and before the user is allowed to crop the
>                     // image by default, but this can cause out-of-memory
> issues
>                     // so we explicitly disable it.
>                     nocrop: true // don't allow the user to crop the image
>                 }
>             });

Re-read the bug I realized that this bug has nothing to do with the cropper, so it doesn't make sense to convert it to what I did above. We should create another bug for doing that.

To work on this bug we first need log to tell what exactly happened. Marking qawanted.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #5)
> (In reply to Jason Smith [:jsmith] from comment #2)
> >             var pick = new MozActivity({
> >                 name: "pick",
> >                 data: {
> >                     type: ["image/png", "image/jpg", "image/jpeg"],
> >                     // In FxOS 1.3 and before the user is allowed to crop the
> >                     // image by default, but this can cause out-of-memory
> > issues
> >                     // so we explicitly disable it.
> >                     nocrop: true // don't allow the user to crop the image
> >                 }
> >             });
> 
> Re-read the bug I realized that this bug has nothing to do with the cropper,
> so it doesn't make sense to convert it to what I did above. We should create
> another bug for doing that.
> 
> To work on this bug we first need log to tell what exactly happened. Marking
> qawanted.

What type of log are you looking for? We could dmesg log if need be.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jason Smith [:jsmith] from comment #6)
> What type of log are you looking for? We could dmesg log if need be.

dmesg, logcat, meminfo. Thank you!

Comment 8

[Ting-Yu Chou [:ting] (away)]

I can reproduce the issue, Browser is killed by LMK when Gallery is in foreground (pick-view):

  <4>0[ 1264.423721] lowmem_shrink select 4146 (Browser), adj 10, size 6207, to kill
  <4>0[ 1264.423739] lowmem_shrink send sigkill to 4146 (Browser), adj 10, size 6207

But this is different from other web activities behavior, which all the processes on the chain are stayed in foreground, not sure why Browser is different?

Comment 9

[Ting-Yu Chou [:ting] (away)]

BTW, bug 982491 groups all the

Comment 10

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #9)
> BTW, bug 982491 groups all the

groups all the app on activities chain in one process, seems Browser is not included.

Comment 11

[Ting-Yu Chou [:ting] (away)]

Fabrice, the web activity started by Browser does no group into the same process, is this expected after bug 982491 landed?

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Ting-Yu Chou [:ting] from comment #11)
> Fabrice, the web activity started by Browser does no group into the same
> process, is this expected after bug 982491 landed?

Yes, because the browser app runs in the parent, and we don't want to group apps in the parent.

Comment 14

[Ting-Yu Chou [:ting] (away)]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> Yes, because the browser app runs in the parent, and we don't want to group
> apps in the parent.

Then what if we keep the Browser process in foreground, just like original web activities behavior?

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Alive, is it possible to make that exception for Browser app (see comment above)?

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Removing qawanted since the cause has been found.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

I don't understand. Due to https://bugzilla.mozilla.org/show_bug.cgi?id=892371 all activity caller is foreground already. Who had changed this behavior ever?

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

OK, the answer is we can't.
Read
https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/system/js/activity_window.js#L160

Unless we want to regress https://bugzilla.mozilla.org/show_bug.cgi?id=914412

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

..And that is a gecko bug which is workaround by gaia side anyway. But AFAIK until now we don't have clue to resolve bug 914412 in gecko side.

Comment 21

[David Flanagan [:djf]]

This is the bug that keeps coming back to bite us!  The browser app is different than all other apps when it initiates a pick activity. For reasons I don't understand, it cannot be forced to stay alive during the pick, so for any memory pressure, it dies.

It might be interesting to know if we have the same problem when picking from the Camera in this case.  If we don't know, then we probably will when we add the code to deal with EXIF rotation.

Tim: unless this can be fixed in the system app, I think this file picker in the browser case is what is going to force us to reduce the size of picked images as I proposed in the recent email thread.

Comment 22

[[:fabrice] Fabrice Desré]

What's special with the browser is that the app runs in the parent, but the tabs are OOP. Since we can't share the tab process that calls mozActivity with the activity provider, we end up killing the tab in some cases.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to David Flanagan [:djf] from comment #21)
> Tim: unless this can be fixed in the system app, I think this file picker in
> the browser case is what is going to force us to reduce the size of picked
> images as I proposed in the recent email thread.

That might not be a proper solution either because even if we managed to save some memory on the picker, heavier pages (e.g. Facebook) will still likely be killed.

That said, I don't have any better ideas on this ...

Comment 24

[Joe Cheng [:jcheng] (please needinfo)]

should we consider removing the "pick image" feature on the browser? if it will almost crash most of the time? 

Ni? Gregor / Marvin

Comment 25

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #24)
> should we consider removing the "pick image" feature on the browser? if it
> will almost crash most of the time? 
> 
> Ni? Gregor / Marvin

I don't think that's possible. The only way we can disable this would be to remove the pick activity directly from the gallery app itself. But we can't do that because that would remove other required functionality by caveat such as being able to attach images to a MMS from the gallery & adding a contact photo from the gallery.

The other problem here is that even if we could isolate this outside of core gaia apps, then disabling this would pretty much remove our chances of having Facebook & Twitter be useful on the Tarako device, as this is common workflow used to share images on those sites.

Comment 26

[Ting-Yu Chou [:ting] (away)]

From what Alive said at comment 19, maybe we should fix it correctly to keep browser and gallery in the same foreground process?

Comment 27

[David Flanagan [:djf]]

Note that Robert's page runs in the browser and uses MozActivity directly. But this bug actually affects any page that uses <input type="file"> to display an HTML file upload element.  I've got a test case at http://djf.net/up.html which demonstrates this.

When I tap on the button, I see a choice of Gallery or Camera to select an image file from. It turns out that the Browser app is killed as soon as Camera or Gallery starts, before I even do the pick. So this has nothing to do with needing to rotate or crop an image that is too big.  Tarako is not even keeping the browser app alive at all. 

The homescreen is staying alive, however, and I think that is the wrong choice in this case. The homescreen should be killed and the browser should stay open so the activity can complete.

I think maybe we should reconsider whatever the tradeoff was in bug 914412 because this bug seems really serious.  I never understood 914412, but I note from the description that it was about view activities. Maybe the patch for 914412 could be modified so it only applied to view activities and did not affect pick activities?

Changing the component for this bug. It is not specific to gallery or camera (or to browser). I think this is going to have to be fixed in the system app or possibly in gecko 

Setting needinfo for Alive to ask if the code in activity_window.js could be modified so that it only uses the 914412 patch for the view activity and not for the pick activity.  Or only for inline activities that do not have returnValue:true.  (Though apps/fl/manifest.webapp which was what bug 914412 was about includes returnValue:true, so we'd have to remove that)

Comment 28

[(inactive after 6/18) Alive Kuo [:alive]]

For tarako project I will try what David suggested. AFAIK system app won't know the activity type(name) when we deal with the request..

Comment 29

[(inactive after 6/18) Alive Kuo [:alive]]

Uh..I just realized if we keep pick activity caller (browser at foreground) we will meet bug 914412 again -- no response when pick activity finishes.

Comment 30

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #29)
> Uh..I just realized if we keep pick activity caller (browser at foreground)
> we will meet bug 914412 again -- no response when pick activity finishes.

Interesting...914412 seems gone even when I put browser app in foreground...

Comment 31

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: patch for v1.3t

Uh..I am not sure why bug 914412 is gone with pick activity.

David, I cannot find your test page http://djf.net/t.html it seems changed, could you test this patch on your side to make sure browser works with view activity?

Comment 32

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8411550 [details] [review]
patch for v1.3t

We can land this on 1.3 if :djf can verify this fixes things.

Comment 33

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8411550 [details] [review]
patch for v1.3t

Zac, I made a change to keyboard.dismiss due to unknown failure when testing test_browser_bookmark.py
Please see https://travis-ci.org/mozilla-b2g/gaia/jobs/23646544 for reference.
Could you verify my change or help me to figure out what's wrong? Thanks.

Comment 34

[Zac C (:zac)]

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #33)
> Comment on attachment 8411550 [details] [review]
> patch for v1.3t
> 
> Zac, I made a change to keyboard.dismiss due to unknown failure when testing
> test_browser_bookmark.py
> Please see https://travis-ci.org/mozilla-b2g/gaia/jobs/23646544 for
> reference.
> Could you verify my change or help me to figure out what's wrong? Thanks.

Alive, yes we recognised and made a similar change to the logic of dismiss() on master a few months ago, but we didn't uplift to branches. On master we just removed the wait on line 285. I think your patch is fine, it effectively does the same thing.

Comment 35

[Zac C (:zac)]

Comment on attachment 8411550 [details] [review]
patch for v1.3t

r+ on the condition that Travis passes.

Comment 37

[David Flanagan [:djf]]

Comment on attachment 8411550 [details] [review]
patch for v1.3t

I can verify that this patch fixes this bug and it does not cause a regression by bringing bug 914412 back.

The patch looks like it just removes the workaround that was added in 914412. The code base has changed, and I do not understand it, so I am not 100% certain that there are not functional changes other than the removal of the workaround.

But it looks good to me. And it fixes a serious bug, so let's get this landed!

We should probably also fix this on 1.4 and on master. Alive: the test page at djf.net/t.html just has more test cases now. From that page, I tested by clicking on "A TCL test suite" and then trying the "Sound", "image" and "smaller_jpeg" links on that new page. Each of those links invokes the view activity and the browser remains responsive even when we return from the activity. I tested by applying your patch to my Tarako, which was last flashed on Friday.

Comment 38

[(inactive after 6/18) Alive Kuo [:alive]]

1.3t https://github.com/mozilla-b2g/gaia/commit/028bc224bc9133c5dc981f931847f5545cd1d8a7

Comment 39

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

verified: 
Gaia      b5adc5a943d3abbd6ab070a47c847f2c24891cc5
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e9890f5d4709
BuildID   20140429014002
Version   28.1
ro.build.version.incremental=eng.cltbld.20140429.052234
ro.build.date=Tue Apr 29 05:22:41 EDT 2014
Tarako

Comment 40

[Jan Jongboom [:janjongboom] (inactive)]

Mike, do you think this will affect 1.4 on Dolphin? Also low-memory and such.

Comment 41

[Mike Lee [:mlee]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #40)
> Mike, do you think this will affect 1.4 on Dolphin? Also low-memory and such.

I believe that's Spreadtrum's decision to make. Preeti please confirm.

Comment 42

[Preeti Raghunath(:Preeti)]

Prefer to take on 1.4 dolphin branch

Comment 43

[Wayne Chang [:wchang]]

I would prefer not to rush this into 1.4 given the time and stage of that branch unless we can confirm this does impact dolphin.

Peipei, Can you find out if it does?

Comment 44

[Peipei Cheng (needinfo if you need my action)]

(In reply to Wayne Chang [:wchang] from comment #43)
> I would prefer not to rush this into 1.4 given the time and stage of that
> branch unless we can confirm this does impact dolphin.
> 
> Peipei, Can you find out if it does?

Dolphin has larger RAM so it behaved a little better. I can upload 1~2 pictures. But then if I upload more or Gallery has many pictures or there are some background apps, the pick activity also fails.

Comment 45

[Ivan Tsay (:ITsay)]

Hi Alive,

Is the patch ok for uplifting to v1.4 or is re-base needed for 1.4?
It seems that there is still broken cases in Dolphin when RAM is running low.

Comment 46

[(inactive after 6/18) Alive Kuo [:alive]]

Yes I think it's safe for v1.4

Comment 47

[Ivan Tsay (:ITsay)]

Comment on attachment 8411550 [details] [review]
patch for v1.3t

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Not regression. Bug found from partner's test
[User impact] if declined: unable to upload image to web when memory is running low. Per comment 44, The issue is still easily hit in Dolphin.
[Testing completed]: Yes, the patch is verified in 1.3T
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A

Comment 48

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8411550 [details] [review]
patch for v1.3t

Approved for 1.4.

Comment 49

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8411550 [details] [review]
patch for v1.3t

It doesn't look like this fix landed on 2.0 or master and we should consider taking it on both of these branches to avoid future uplift requests. I'm not approving for those branches in case there is a valid reason to restrict this patch to Tarako and Dolphin.

Alive - Can you please comment on whether we can/should take this change on master. If so, can you please land it.

See comment 47 for the approval request.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

This appears to need some serious rebasing for v1.4 uplift.

Comment 51

[Lawrence Mandel [:lmandel] (use needinfo)]

Alive, Ivan ^

Comment 52

[(inactive after 6/18) Alive Kuo [:alive]]

I will make patches for 1.4 and later branch.

Comment 53

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: patch for 1.4

Comment 54

[Ivan Tsay (:ITsay)]

Comment on attachment 8449941 [details] [review]
patch for 1.4

Hi Lawrence,

Thanks for the ni? re-request for the approval on 1.4 patch.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Not regression. Bug found from partner's test
[User impact] if declined: unable to upload image to web when memory is running low. Per comment 44, The issue is still easily hit in Dolphin.
[Testing completed]: Yes, the patch is verified in 1.3T
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A

Comment 55

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8449941 [details] [review]
patch for 1.4

Approved for 1.4.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/3cfbafe1a68cb5366bb498128951c3a99d539ce7

Comment 57

[bhavana bajaj [:bajaj]]

Still waiting for master landing and branch patch for 2.0 here from alive.

Comment 58

[(inactive after 6/18) Alive Kuo [:alive]]

For master and v2.0, the code had been remove by bug 1024168 so nothing to do here.

Comment 59

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 1.4, 2.0, 2.1.

Result: The selected photo is uploaded and displayed on the webpage.

Device: Flame 1.4 (319mb)(Jelly Bean Base)(Shallow Flash)
BuildID: 20140922000333
Gaia: efa2b8cb095407df942fee7732a5547c7034ef9b
Gecko: 02154a103d43
Version: 30.0 (1.4)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Device: Flame 2.0 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141029000205
Gaia: 9f5b6f025e528fabfcc068782cb9b492cb51a7f9
Gecko: de8cfd54bf93
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141029001202
Gaia: eb0aab0f13c78c7ac378ad860e865c4b6eaf669f
Gecko: 318019f80a8e
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0