Closed Bug 996141 Opened 7 years ago Closed 7 years ago

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/

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3T+, b2g-v1.3 unaffected, b2g-v1.3T verified, b2g-v1.4 verified, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S1 (9may)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- unaffected
b2g-v1.3T --- verified
b2g-v1.4 --- verified
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: jmitchell, Assigned: alive)

References

Details

(Keywords: perf, Whiteboard: [c=memory p= s=2014.05.09.t u=tarako] 1.3tarakorun2[sprd294158], OOM)

Attachments

(4 files)

Attached file 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
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.
blocking-b2g: --- → 1.3T?
Component: Gaia::Browser → Gaia::Gallery
triage: 1.3T+ for broken use case
blocking-b2g: 1.3T? → 1.3T+
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.
Assignee: nobody → johu
Flags: needinfo?(johu)
Summary: [B2G][Tarako][Browser] 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/ → [B2G][Tarako][Browser] Make image cropper in the gallery picker configurable
Flags: needinfo?(johu)
Summary: [B2G][Tarako][Browser] Make image cropper in the gallery picker configurable → [B2G][Tarako][Gallery] Make image cropper in the gallery picker configurable
(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.
Assignee: johu → nobody
Keywords: qawanted
Summary: [B2G][Tarako][Gallery] Make image cropper in the gallery picker configurable → 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/
(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.
(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!
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?
BTW, bug 982491 groups all the
See Also: → 982491
(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.
Fabrice, the web activity started by Browser does no group into the same process, is this expected after bug 982491 landed?
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
Duplicate of this bug: 997641
(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?
Alive, is it possible to make that exception for Browser app (see comment above)?
Flags: needinfo?(alive)
Removing qawanted since the cause has been found.
Keywords: qawanted
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?
Flags: needinfo?(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.
Duplicate of this bug: 998075
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.
Flags: needinfo?(timdream)
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.
(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 ...
Flags: needinfo?(timdream)
Whiteboard: 1.3tarakorun2 → 1.3tarakorun2[sprd294158]
Whiteboard: 1.3tarakorun2[sprd294158] → 1.3tarakorun2[sprd294158], OOM
should we consider removing the "pick image" feature on the browser? if it will almost crash most of the time? 

Ni? Gregor / Marvin
Flags: needinfo?(mkhoo)
Flags: needinfo?(anygregor)
(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.
Flags: needinfo?(mkhoo)
Flags: needinfo?(anygregor)
From what Alive said at comment 19, maybe we should fix it correctly to keep browser and gallery in the same foreground process?
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)
Component: Gaia::Gallery → Gaia::System
Flags: needinfo?(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..
Assignee: nobody → alive
Flags: needinfo?(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.
(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...
Attached file 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?
Attachment #8411550 - Flags: review?(timdream)
Attachment #8411550 - Flags: feedback?(dflanagan)
Comment on attachment 8411550 [details] [review]
patch for v1.3t

We can land this on 1.3 if :djf can verify this fixes things.
Attachment #8411550 - Flags: review?(timdream)
Attachment #8411550 - Flags: review?(dflanagan)
Attachment #8411550 - Flags: feedback?(dflanagan)
Attachment #8411550 - Flags: feedback+
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.
Attachment #8411550 - Flags: feedback?(zcampbell)
(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 on attachment 8411550 [details] [review]
patch for v1.3t

r+ on the condition that Travis passes.
Attachment #8411550 - Flags: feedback?(zcampbell) → feedback+
Duplicate of this bug: 996293
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P1
Whiteboard: 1.3tarakorun2[sprd294158], OOM → [c=memory p= s= u=tarako] 1.3tarakorun2[sprd294158], OOM
Whiteboard: [c=memory p= s= u=tarako] 1.3tarakorun2[sprd294158], OOM → [c=memory p= s= u=tarako] 1.3tarakorun2[sprd294158], OOM, [ETA: 4/29]
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.
Attachment #8411550 - Flags: review?(dflanagan) → review+
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
Severity: normal → blocker
Whiteboard: [c=memory p= s= u=tarako] 1.3tarakorun2[sprd294158], OOM, [ETA: 4/29] → [c=memory p= s=2014.05.09.t u=tarako] 1.3tarakorun2[sprd294158], OOM, [ETA: 4/29]
Target Milestone: --- → 2.0 S1 (9may)
Mike, do you think this will affect 1.4 on Dolphin? Also low-memory and such.
Flags: needinfo?(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.
Flags: needinfo?(mlee) → needinfo?(praghunath)
Prefer to take on 1.4 dolphin branch
Flags: needinfo?(praghunath)
Whiteboard: [c=memory p= s=2014.05.09.t u=tarako] 1.3tarakorun2[sprd294158], OOM, [ETA: 4/29] → [c=memory p= s=2014.05.09.t u=tarako] 1.3tarakorun2[sprd294158], OOM, [ETA: 4/29][dolphin land]
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?
Flags: needinfo?(pcheng)
(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.
Flags: needinfo?(pcheng)
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.
Flags: needinfo?(alive)
Yes I think it's safe for v1.4
Flags: needinfo?(alive)
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
Attachment #8411550 - Flags: approval-gaia-v1.4?(lmandel)
Comment on attachment 8411550 [details] [review]
patch for v1.3t

Approved for 1.4.
Attachment #8411550 - Flags: approval-gaia-v1.4?(lmandel) → approval-gaia-v1.4+
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.
Attachment #8411550 - Flags: approval-gaia-v2.0?(release-mgmt)
Flags: needinfo?(alive)
This appears to need some serious rebasing for v1.4 uplift.
Whiteboard: [c=memory p= s=2014.05.09.t u=tarako] 1.3tarakorun2[sprd294158], OOM, [ETA: 4/29][dolphin land] → [c=memory p= s=2014.05.09.t u=tarako] 1.3tarakorun2[sprd294158], OOM, [dolphin land]
Alive, Ivan ^
Flags: needinfo?(itsay)
I will make patches for 1.4 and later branch.
Flags: needinfo?(alive)
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
Attachment #8449941 - Flags: approval-gaia-v1.4?(lmandel)
Flags: needinfo?(itsay)
Comment on attachment 8449941 [details] [review]
patch for 1.4

Approved for 1.4.
Attachment #8449941 - Flags: approval-gaia-v1.4?(lmandel) → approval-gaia-v1.4+
v1.4: https://github.com/mozilla-b2g/gaia/commit/3cfbafe1a68cb5366bb498128951c3a99d539ce7
Whiteboard: [c=memory p= s=2014.05.09.t u=tarako] 1.3tarakorun2[sprd294158], OOM, [dolphin land] → [c=memory p= s=2014.05.09.t u=tarako] 1.3tarakorun2[sprd294158], OOM
Still waiting for master landing and branch patch for 2.0 here from alive.
Flags: needinfo?(alive)
For master and v2.0, the code had been remove by bug 1024168 so nothing to do here.
Flags: needinfo?(alive)
Attachment #8411550 - Flags: approval-gaia-v2.0?(release-mgmt) → approval-gaia-v2.0-
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.