935273 - [B2G][Browser] Some web pages are reloaded after using the Gallery picker to upload images

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Matthew Vaughan]

Attachment: Video of Facebook reloading after picking an image to upload.

Description:
Some web pages that allow the user to upload an image to the website are reloaded after the user selects an image using the Gallery picker. This was tested with the mobile version of Facebook and Twitter, and the desktop version of Flickr, all reloading after picking an image.

Repro Steps:
1) Update Buri to v1.2 COM RIL BuildID: 20131103004003
2) If needed, take a picture using the Camera app
3) Launch the Browser
4) Navigate to Facebook (www.facebook.com)
5) Sign into a test Facebook account
6) Select "Photo" at the top of the page to upload an image
7) Select "Browse" > "Gallery" > pick an image
8) Comfirm the image selection (box with a check mark)

Actual:
Facebook page reloads to the page that first appeared when signing in.

Expected:
The chosen image displays on the webpage where the user can then post it onto Facebook.

Environmental Variables:
Device: Buri v1.2 COM RIL
BuildID: 20131103004003
Gaia: cb981e2f47bc644b4d178d54378c3676c946facf
Gecko: eec4da1b27eb
Version: 26.0
RIL Version: 01.01.00.019.276

Notes:
Repro frequency: 5/5
Link to failed test case: https://moztrap.mozilla.org/manage/case/8909/
See attached: PageReload.3gp & LogCat

Comment 1

[Matthew Vaughan]

Attachment: LogCat of attempting to upload an image to Facebook.

Comment 2

[Matthew Vaughan]

This bug does NOT reproduce on the 11/01/13 1.1 build. The chosen image can be uploaded on the first try and it does not reload the web page.

Environmental Variables:
Device: Leo 1.1 COM RIL
BuildID: 20131101041316
Gaia: 39b0203fa9809052c8c4d4332fef03bbaf0426fc
Gecko: 31fa87bfba88
Version: 18.0
RIL Version: 01.01.00.019.276

Comment 3

[Andrew Overholt [:overholt]]

jwatt/baku, any thoughts here?  Could this be related to file input types?

Comment 4

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98bf1daa9e90

Comment 5

[Andrea Marchesini [:baku]]

wrong bug... sorry.
Btw: No ideas yet. I'll try to reproduce it.

Comment 6

[Jonathan Watt [:jwatt]]

FWIW I don't know of anything that would cause this either. Hopefully baku can repro.

Comment 7

[(no longer active)]

Could it be that the browser process is getting OOM killed when you're using the Gallery app?

Comment 8

[(no longer active)]

(In reply to comment #7)
> Could it be that the browser process is getting OOM killed when you're using
> the Gallery app?

(Note that this is just my guess, I have no evidence to support it!)

Comment 9

[Nicholas Nethercote [inactive]]

(Please re-add the Memshrink tag once you're more certain this is OOM related :)

Comment 10

[Jason Smith [:jsmith]]

QA Wanted - Can someone get a dmesg log here?

Reference to follow here - https://wiki.mozilla.org/B2G/Debugging_OOMs#Step_1:_Verify_that_it.27s_actually_an_OOM.

Comment 11

[Matthew Vaughan]

I tested with the mobile version of Facebook and Tumblr, and the desktop version of Flickr. The dmesg logs all seem to indicate an OOM issue since I am seeing "sigkill" for the Browser app for all three websites.

I have attached the dmesg logs for all three websites.
Facebook dmesg.txt
Flickr dmesg.txt
Tumblr dmesg.txt

Comment 12

[Matthew Vaughan]

Attachment: Facebook dmesg

Comment 13

[Matthew Vaughan]

Attachment: Flickr dmesg

Comment 14

[Matthew Vaughan]

Attachment: Tumblr dmesg

Comment 15

[bhavana bajaj [:bajaj]]

NI on :overholt to help with assignee here.

Comment 16

[Preeti Raghunath(:Preeti)]

Milan,

Please check if this is gallery related?

Comment 17

[[:fabrice] Fabrice Desré]

See bug 921659 for general issue about memory usage regression. That's likely to play a role here.

Comment 18

[Andrew Overholt [:overholt]]

Gregor suggested a memory profile could help track this down.  Mike, can someone on the perf team help with that?

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Preeti Raghunath(:Preeti) from comment #16)
> Milan,
> 
> Please check if this is gallery related?

As in, the gallery using too much memory, if it is OOM?  I'm not sure this is related to graphics.

Comment 20

[Mike Lee [:mlee]]

Hema,

Please assign someone on the Gallery team to look into this. Debugging instructions are available here: https://wiki.mozilla.org/B2G/Debugging_OOMs

Comment 21

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

Note: This doesn't happen when facebook is bookmarked to homescreen.
In other words, the reloading after activity is done only occurs when facebook is running at browser app but not happen at system app.

Comment 22

[Preeti Raghunath(:Preeti)]

Gregor,

Can you please check if this bug is related to browser? Please re-assign appropriately.

Comment 23

[David Flanagan [:djf]]

The pick activity decodes the selected image at full size before returning it to the requesting app as part of allowing the user to crop the image.

My guess is that bug is a simple OOM consequence of increasing the camera resolution from 2mp to 5mp. With a 5mp image, the pick activity allocates 20mb to display the picked image, and the browser crashes.

If we can skip the cropping step (the code that initiates the activity should pass 'nocrop: true' in the activity request) then Gallery could be modified so that it shows the image preview rather than the fullsize image, and the memory usage would be much lighter.

Currently the Gallery always creates an ImageEditor object that decodes the fullsize image, even in the nocrop case. We'd have to modify the gallery to handle the no crop case with a MediaFrame object instead of an ImageEditor object.

Unless someone thinks that there is some other underlying cause for this, I think we need to fix the OOM by using less memory.

Gregor: if you can have someone on your team change the code (in the browser or system app) that initiates the pick activity so that it adds the nocrop flag, then I can modify the gallery so that it doesn't use so much memory in that case.

Comment 24

[Gregor Wagner [:gwagner]]

Dale, can you help out here?

Comment 25

[Matthew Vaughan]

This issue looks to have started on the 9/12/13 1.2 build. It was very difficult finding a regression window due to the browser constantly crashing when attempting to upload an image to Facebook, Flickr, etc. on builds between 9/01/13 and 9/12/13.

- Works -
Environmental Variables:
Device: Buri v1.2 MOZ RIL
BuildID: 20130901040215
Gaia: 9fb5802df60a9081846d704def01df814ed8fbd4
Gecko: b6c29e434519
Version: 26.0a1
Firmware Version: 20131104

- Broken -
Environmental Variables:
Device: Buri v1.2 MOZ RIL
BuildID: 20130912040201
Gaia: 9ffd2899eb91388f7fc1ce6f7a895a6f5f922c05
Gecko: a98569f21abe
Version: 26.0a1
Firmware Version: 20131104

Comment 26

[Jason Smith [:jsmith]]

Can you go deeper with that regression range by trying different workflows to ensure a crash doesn't happen?

Comment 27

[Alex Keybl [:akeybl]]

(In reply to David Flanagan [:djf] from comment #23)
> Gregor: if you can have someone on your team change the code (in the browser
> or system app) that initiates the pick activity so that it adds the nocrop
> flag, then I can modify the gallery so that it doesn't use so much memory in
> that case.

What's the bug # for this work? There's a missing nocrop blocker here..

Comment 28

[Ben Francis [:benfrancis]]

FYI, having watched the video this looks very much like the browser tab containing Facebook is getting OOM killed. The browser will reload a killed tab when you switch back to it.

David, I'm not sure where the pick activity gets initiated but it's not in the browser app code. I suspect maybe in the file picker in Gecko?

Comment 29

[Matthew Vaughan]

I attempted to reduce the regression window but I was unsuccessful at doing so. I checked every available build between 9/01/13 and 9/12/13. Also, while testing with different areas to upload from, I tested using bookmarks on the Homescreen (from the Browser and e.me) and installing different apps (like Facebook, Twitter, etc.) from the Marketplace.

On the 9/02-9/04 builds, the image would successfully upload when using the Camera or Wallpaper option. The OS or Browser would crash when using the Gallery option however. 

On the 9/05-9/11 builds, the OS or Browser would crash when using the Camera, Gallery, or Wallpaper option to upload an image.

So unfortunately the regression window from comment 25 stills stands since images would upload just fine when using any of the options on the 9/01 build, but on the 9/12 and later the web page will reload when attempting to upload an image using the Gallery option.

Attached dmesg_log.txt

Comment 30

[Matthew Vaughan]

Attachment: Dmesg log from the 11/19/13 1.2 build

Comment 31

[David Flanagan [:djf]]

(In reply to Ben Francis [:benfrancis] from comment #28)

> David, I'm not sure where the pick activity gets initiated but it's not in
> the browser app code. I suspect maybe in the file picker in Gecko?

I suspect so too, but am hoping that someone from the System team can find out where that is and add the nocrop flag, either as part of this bug or under another koi+ bug.

Gregor asked Dale to help above bug didn't set needinfo for Dale.  So I'll set needinfo on both Dale and Ben in case one of them can take it.

Comment 32

[Dale Harvey (:daleharvey)]

Looks like http://mxr.mozilla.org/mozilla-central/source/b2g/components/FilePicker.js is responsible for tge activity, can take a look at trying the nocrop thing

Comment 33

[Andrea Marchesini [:baku]]

Attachment: crop.patch

I didn't see this bug was assigned to you and, working on bug 923274, I was touching FilePicker.jsm code. This patch is not tested, but maybe it works and maybe it's a good starting point.

Comment 34

[Dale Harvey (:daleharvey)]

Comment on attachment 8336004 [details] [diff] [review]
crop.patch

Its cool I only seen this bug / took it a few hours ago and hadnt got to it

Will be waiting quite a while for my build to finish to test this, but codewise looks perfect, not sure theres anything we can do to test this specific crash (we cant test against facebook)

Got a follow up comment as well

Comment 35

[Dale Harvey (:daleharvey)]

I realise although it will be a bigger gaia patch but David do you think it might be a cleaner api to switch to nocrop by default, in the sense that 'pick' by default does the least amount possible and additional features are opted in via arguments, it seems cleaner / more future proof to me, also generally worry about us baking in API details into gecko

Comment 36

[Dale Harvey (:daleharvey)]

Also baku since you have the patch here, want to take the bug? unless it turns into a gaia patch then I think this will be good, probably want fabrice to review though

Comment 37

[Ben Francis [:benfrancis]]

What Dale said.

Comment 38

[Dale Harvey (:daleharvey)]

Hmm, patch doesnt stop the tab from refreshing, thinking that this isnt an oom killer issue 

http://pastebin.mozilla.org/3647549

Comment 39

[:gerard-majax]

I did a couple of tests on current gecko/gaia master (updated today). Use case is uploading a picture from the gallery to my facebook account:
 - using the Inari, I reproduce the exact error ;
 - using HTC Desire Z, I can upload sucessfully the same pictures as the Inari.

The test picture for this is a JPEG file ~200k.

Comment 40

[Dale Harvey (:daleharvey)]

So I am running a debug build so I can get more information about whats triggering the oom, as it seems it must be that, however I missed the part of Davids comment about also needing a gallery side fix, will be able to test with that.

Will switch bug to djf for the moment

Comment 41

[David Flanagan [:djf]]

(In reply to Alexandre LISSY :gerard-majax from comment #39)

> The test picture for this is a JPEG file ~200k.

Would you tell me the resolution of the photo?  The file size isn't relevant to the memory usage.

Comment 42

[David Flanagan [:djf]]

(In reply to Dale Harvey (:daleharvey) from comment #35)
> I realise although it will be a bigger gaia patch but David do you think it
> might be a cleaner api to switch to nocrop by default, in the sense that
> 'pick' by default does the least amount possible and additional features are
> opted in via arguments, it seems cleaner / more future proof to me, also
> generally worry about us baking in API details into gecko

Yes, I think that would be cleaner. But I also think that it is not reasonable to change the default behavior of the activity for a koi+ bug.  That would break too many other apps.

Maybe I need to change the pick activity flow so that it just shows a low-resolution (low-memory) preview by default and the user has to tap a button to enter (high-memory usage) crop mode.  Or maybe for the crop-only use case I'm doing here, I can modify my ImageEditor code so that I can pass the preview image to it.  This would mean that it wouldn't have to use a lot of memory unless the user actually cropped the image.  It will be kind of ugly, and not a trivial patch, but I think I can fix this.

So no gecko changes required. Andrea: Dale is right: please don't set nocrop in the gecko changes you are working on.

Comment 43

[:gerard-majax]

(In reply to David Flanagan [:djf] from comment #41)
> (In reply to Alexandre LISSY :gerard-majax from comment #39)
> 
> > The test picture for this is a JPEG file ~200k.
> 
> Would you tell me the resolution of the photo?  The file size isn't relevant
> to the memory usage.

1600x1200. Those pics were taken in the plane back from Oslo with the Inari :)

Comment 44

[David Flanagan [:djf]]

I assume that this bug is related to bug 939962, and in particular https://bugzilla.mozilla.org/show_bug.cgi?id=939962#c23

The difference is that for this bug I ought to be able to avoid decoding the picked image if the user doesn't crop, so that should save memory and prevent the OOM even if the underlying bug does not get fixed.

Comment 45

[David Flanagan [:djf]]

On my Buri running master, if I log into mobile.twitter.com and go to attach a photo, I find that the browser process is killed when Gallery starts up.  I was going to try to save memory during the crop process by not decoding the full-size photo if that is not necessary.

But it turns out that the OOM happens before any large image is decoded.

It may still be worth fixing (in a separate bug) the issue where images are decoded even if they are not cropped.  Because that will save a lot of memory.  But it won't fix this bug: there is something else nasty going on here.

Setting regressionwindow-wanted and needinfo on Matthew: could you find us a regression range here? 

Milan: could this be related to the gfx changes you mention in https://bugzilla.mozilla.org/show_bug.cgi?id=939962#c23

Comment 46

[David Flanagan [:djf]]

Hmm. Now I can't reproduce what I saw above.  The last two times I've tried, the Browser does not OOM until I actually start the crop.

So I will proceed with my patch to reduce the memory used by cropping.

I still think it is worth finding a regression range on this.

Comment 47

[Matthew Vaughan]

Hey David, 

I attempted to get a regression window for this issue (comment 25), but due to an issue where the Browser simply crashes right after I select where I want to get an image from, the window is kind of big. Comment 29 explains my findings further. Is there something else that you need me to find here, or will what's in comment 25 sufficient?

Comment 48

[David Flanagan [:djf]]

Attachment: link to patch on github

This attachment is a gaia patch that keeps memory usage low when the user does not crop the picked image.

With this patch applied, I can upload an image to twitter if I don't crop it. The browser still crashes if I do crop it.

The patch probably isn't ready for review yet, but I'm posting it here so I can ask for more testing: does this patch make uncropped image uploads to facebook work, for example?

The patch is against master. I'm not sure if it would apply cleanly to 1.2, so you'll need to test against 1.3 for now.

Obviously it is not enough to prevent the browser crash only in the no cropping case.  We either need to apply Andrea's patch to turn off cropping. (Not ideal, but nice to know we can do it if necessary) or we need to get to the bottom of why we are seeing so many OOMs on Buri devices.)

I still think there is some other Buri bug causing increased memory usage and OOMs.

Comment 49

[David Flanagan [:djf]]

Matthew,

Are you able to try out the attached gaia patch?  Does it fix the issue for you (if you do not crop the image you pick?)

Comment 50

[David Flanagan [:djf]]

In my testing, when the gallery app starts up to pick an image, it ends up with a VSIZE of ~115k.  (using adb shell b2g-ps)

If I crop the image slightly and click Done (which forces a full-size image decode) the VSIZE shoots up to 165k.  If I'm interpreting that right, that is an extra 50mb of memory.  Just to decode a 3mp image.  I'd expect VSIZE to go up 12 mb for the decode, not 50mb.  

What's going wrong here? Anyone up for doing a memory analysis on this?

Comment 51

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to David Flanagan [:djf] from comment #45)
> ...
> 
> Milan: could this be related to the gfx changes you mention in
> https://bugzilla.mozilla.org/show_bug.cgi?id=939962#c23

Doubt it, the regression window is too early.

Comment 52

[David Flanagan [:djf]]

When trying to pick an image from Gallery using the Contacts app, I no longer see what I saw in comment #50.  Just 12+mb memory used.  (Though if I click back and then select the image again it allocates another 12mb. The old memory doesn't get freed for a while. But there is no memory pressure at the time.)

Nothing interesting in the about_memory.py report that I can see.  We may just be running out of memory...

I should look at how much memory the browser app is using when it visits twitter, flickr, etc. If big websites are using more memory, maybe that is the issue.

I've really got no clue here.

Comment 53

[David Flanagan [:djf]]

I was thinking that maybe this had something to do with large heavy websites like twitter...  Maybe big websites made the browser app take more memory.  But I can reproduce this OOM issue even with the lightest possible site that uses a fileupload element.  My test case is at http://djf.net/f.html

I have one 3megapixel image on my sdcard. When the gallery is launched, Browser is using 66mb and Gallery is using 80.  When I tap on the image it is decoded at full size (which allocates 12mb for the image data) and displayed with a crop overlay. (I'm not using either of the patches attached to this bug) At this point the Browser app crashes and Gallery's memory usage increases to 149mb.

I don't see anything obviously interesting in the memory report, but I don't really know how to read them. I'll attach the report next.

Comment 54

[David Flanagan [:djf]]

Attachment: memory-reports

Comment 55

[David Flanagan [:djf]]

I can also reproduce the OOM when selecting an image from the camera, but only if I double-tap on the preview image to zoom in.

When the camera app first opens up, Browser uses 68mb and Camera uses 80mb.

Then I take a picture, and it shows me the preview. Camera uses 101mb.  (There appears to be a bug: I think it may be showing me the full-size image instead of the preview).

If I double-tap on the image to zoom in, Browser crashes.  Camera's memory remains at 101mb. There must have been some kind of memory spike from the zooming to cause the crash even though I don't think there were any other big image decodes happening.

Comment 56

[David Flanagan [:djf]]

I've confirmed part of the regression window:

In the 9/12, 9/14 and 10/01 builds, I get an OOM, just as I do with the 11/25 build.  (All on mozilla-central, though back in september that was version 26, not 28 as it is today)

In the 9/01, 9/06, 9/10, and 9/11 builds, the browser does not OOM, but the phone crashes as soon as the activity returns from gallery to browser.

So something changed in the 9/12 build.  But even before that we still couldn't pick an image.

Comment 57

[David Flanagan [:djf]]

08/01 and 08/15 builds crash the phone when we try to pick an image.

Bug 912134 landed about 9/12 and looks like it might have a graphics-related memory impact. But I think it landed later in the day than the nightly build where I saw the earliest OOM.

Comment 58

[David Flanagan [:djf]]

Bug 911391 looks like it was a big graphics change that landed on 9/11. I wonder if it had anything to do with the switch from crashing to OOMing that we see in the 9/12 nightly.

Comment 59

[David Flanagan [:djf]]

The 07/01 build does not crash or OOM. The file pick does not seem to complete successfully, however, and I see this in the console: 

E/GeckoConsole(  569): [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIMIMEService.getFromTypeAndExtension]" {file: "jar:file:///system/b2g/omni.ja!/components/FilePicker.js" line: 164}]

There is a 7/22 change to FilePicker.js. Next I should check whether the crash started when that change landed.

Comment 60

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to David Flanagan [:djf] from comment #58)
> Bug 911391 looks like it was a big graphics change that landed on 9/11. I
> wonder if it had anything to do with the switch from crashing to OOMing that
> we see in the 9/12 nightly.

That bug was supposed to only affect JB phones, and this seems to be showing up on Buri which is ICS?

Comment 61

[David Flanagan [:djf]]

(In reply to Milan Sreckovic [:milan] from comment #60)
> (In reply to David Flanagan [:djf] from comment #58)
> > Bug 911391 looks like it was a big graphics change that landed on 9/11. I
> > wonder if it had anything to do with the switch from crashing to OOMing that
> > we see in the 9/12 nightly.
> 
> That bug was supposed to only affect JB phones, and this seems to be showing
> up on Buri which is ICS?

I can't ever keep track of the Android code names, so I don't even know which one is which. I saw mention of testing on both JB and ICS in this comment https://bugzilla.mozilla.org/show_bug.cgi?id=911391#c65 so I wondered. But don't actually know anything about it.  (And in any case, a browser OOM is better than a reboot, so things are different in the 9/12 nightly, but different in a better way :-)

Comment 62

[David Flanagan [:djf]]

I tried the 7/24 nightly, but the FilePicker.js patch that landed 7/22 didn't fix the error there. I hypothesize that the JS error is preventing the photo from being transferred back to the browser and therefore preventing whatever was causing the crash.

Looking again at today's nightly build, however, I realize that the OOM that is the subject of this bug occurs when gallery decodes the image at full size. That is, it is happening before activities or the file picker are involved.  So chasing down what was causing a crash months ago isn't helping to move this forward.

The Contacts app can pick a photo from the gallery without getting an OOM, and Contacts actually starts off with more memory usage than Browser does. So why is Browser getting killed when Contacts is not?

I think I remember seeing something recently in the system app to prevent the LMK from killing the app that launched the current inline activity. I'll go look for that... perhaps Browser is somehow excluded from it.

Comment 63

[David Flanagan [:djf]]

Actually, I can cause browser to OOM by just opening the gallery (not as an activity) and trying to edit a photo.  There must be a big spike in memory when webgl starts up or something...

After browser OOMs, I can start it again, and browser and gallery will coexist.

I've been doing this recent testing with today's nightly build of master, but I suppose I should try it with the 1.2 build instead.

Comment 64

[David Flanagan [:djf]]

Okay, I've figured out what changed on 9/11 to make these OOMs start happening on 9/12.

That was the day that the fix for bug 914412 landed. (Ironically, I actually filed that bug). 914412 was also koi+ and Alive's patch for it very clearly warns of the danger of Browser (but not other apps) getting OOMs when activities are running.

Now this has come back to bite us.

Reverting 914412 makes it possible to pick an image from gallery without the browser OOMing.  But it is not reliable enough. It does still OOM sometimes.

Here's what I think we need to do:

1) Find a different fix for bug 914412 that does not have this browser OOM side-effect.  Setting needinfo on Alive and Tim since they were involved in bug 914412.

2) Figure out why the Gallery's ImageEditor is using up so much memory on Buri devices. If we can get bug 939962 fixed, I'm hoping that will help here also.

3) Land baku's patch that is attached here so that we don't allow image cropping when images are selected by file picker.  And also land my patch here that keeps memory usage low in the no-crop case. I think I should also add in the minor patch I attached bug 939962 since it is also related to reducing memory usage while editing images. My patch here was just a work in progress to test out, and it may still need work. But I'm asking John to review it now because I'll be OOO until next week for the US Thanksgiving holiday.

Comment 65

[David Flanagan [:djf]]

Comment on attachment 8337139 [details] [review]
link to patch on github

John,

This patch may still be rough, but please do at least a preliminary review if you can.

Comment 66

[Sotaro Ikeda [:sotaro]]

> 2) Figure out why the Gallery's ImageEditor is using up so much memory on
> Buri devices. If we can get bug 939962 fixed, I'm hoping that will help here
> also.

SkiaGL might affect to this. SkiaGL require more RAM and Hamachi/Buri have only scare RAM. From them, oom could be easily happen when canvas2d/WebGL is used in app.

Comment 67

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Let me know if the MemShrink team can help here.

Comment 68

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

My proposal to Memory Management is
(1) For long term I want to implement https://bugzilla.mozilla.org/show_bug.cgi?id=892371
    It is to say, promote the process priority for members who are in the activity chaining.
    I filed this long time ago but Fabrice is so busy and this one is not a trivial change.

(2) For short term I'd like gaia system has the ability to change process priority directly in any timing.
    Now we're able to do this by adding mozapptype=critical but it only applies before the mozbrowser iframe is rendered. Alan could you confirm? If we could change mozapptype even when the browser is already there in the DOM tree and let ProcessPriorityManager know the change and ignore docShell.active = false(i.e., background state), we may be able to get rid of this temporarily.

Comment 69

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

Kanru, bug 914412 is the bug to resolve browser app couldn't get focus again in some cases... and I applies some magic: flip page visibility state to make focus work again. Now this seems a pain. What's your analysis to the focus breakage?

Comment 70

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

If bug 914412 is indeed about bug 914412 comment 12, maybe we should consider postMessage() between Browser app and System app instead ... it's still an evil hack, but at least it would not generate OOM side-effect.

Comment 71

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #70)
> If bug 914412 is indeed about bug 914412 comment 12, maybe we should
> consider postMessage() between Browser app and System app instead ... it's
> still an evil hack, but at least it would not generate OOM side-effect.

Please disregard my comment here because Alive told me this will not work.

Comment 72

[Hema Koka [:hema]]

Based on Alive's comment 68, adding Fabrice for input.

Comment 73

[[:fabrice] Fabrice Desré]

(In reply to Hema Koka [:hema] from comment #72)
> Based on Alive's comment 68, adding Fabrice for input.

I don't have much to add unfortunately. We need to fix bug 892371 but that's unlikely that I can work on that soon - so anyone feel free to steal. I don't like option (2) and that will not be easier to implement than (1) I think.

Comment 74

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

Comment on attachment 8337139 [details] [review]
link to patch on github

Sorry for late reviewing. This is a nice patch. But I found a regression when this patch is applied. Please find them at github.

Comment 75

[[:fabrice] Fabrice Desré]

Comment on attachment 8336004 [details] [diff] [review]
crop.patch

Review of attachment 8336004 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed

::: b2g/components/FilePicker.js
@@ +120,5 @@
>         detail.type = this.mFilterTypes;
>      }
>  
> +    for (let prop in this.mExtraProps) {
> +      detail[prop] = this.mExtraProps[prop];

We should check that we don't override an existing property here. bad things may happen if we have a mExtractProps['type'] = ...

Comment 76

[David Flanagan [:djf]]

Andrea,

Would you update your patch to address Fabrice's review nit, and then land it (or set checkin-needed?)

Comment 77

[Andrea Marchesini [:baku]]

Attachment: crop.patch

Comment 78

[David Flanagan [:djf]]

John,

I've update the patch with fixes for the bugs you identified and have asked for another review.

Also, I've fixed it so that the image is displayed correctly if the phone is rotated while cropping.

And I've fixed a bug where small images (that don't have previews) can be picked and cropped.

Comment 79

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

Comment on attachment 8337139 [details] [review]
link to patch on github

Nice patch. It works. r=me.

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ed9645b3d6ec

Comment 81

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed9645b3d6ec

Comment 82

[David Flanagan [:djf]]

Reopening, since the Gaia patch still needs to land

Comment 83

[David Flanagan [:djf]]

Andrea,

I'm not sure how the HTML <input type="file"> element maps uses the FilePicker API.  Will your patch prevent image cropping for any HTML file chooser, or only for those that have an explicit accept="image/*"?  That is, when gecko sees <input type="file"> with no accept attribute, does it set the filterMask to -1 (all bits set) or to 0?

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6b4cf17f3ce3

Please set status-b2g-v1.2 to fixed when the Gaia patch is uplifted to v1.2.

Comment 85

[Ryan VanderMeulen [:RyanVM]]

If the PR is ready to merge, please put checkin-needed on the bug.

Comment 86

[Andrea Marchesini [:baku]]

(In reply to David Flanagan [:djf] from comment #83)
> Andrea,
> 
> I'm not sure how the HTML <input type="file"> element maps uses the
> FilePicker API.  Will your patch prevent image cropping for any HTML file
> chooser, or only for those that have an explicit accept="image/*"?  That is,
> when gecko sees <input type="file"> with no accept attribute, does it set
> the filterMask to -1 (all bits set) or to 0?

With this patch we set 'nocrop' only when accept="image/*" is set. Probably it's better to add it when no 'accept' filters are set. Follow up?

Comment 87

[Andrea Marchesini [:baku]]

Attachment: filterAll.patch

This is the patch that adds nocrop if no filters are set.

Comment 88

[David Flanagan [:djf]]

Comment on attachment 8341775 [details] [diff] [review]
filterAll.patch

Review of attachment 8341775 [details] [diff] [review]:
-----------------------------------------------------------------

A simple followup to the previous patch.

Comment 89

[David Flanagan [:djf]]

Thanks, Andrea!

Setting checkin-needed for the patch in attachment 8341775 [details] [diff] [review].  Also adding [KEEP OPEN] to the whiteboard because the gaia patch has not landed yet.

Comment 90

[David Flanagan [:djf]]

The Gaia patch has an r+ and a green Travis build, but I cannot land it because the Gaia tree is closed.

Comment 91

[David Flanagan [:djf]]

Attachment: gaia patch for the v1.2 branch

The gaia patch does not uplift cleanly to the v1.2 branch. This pull request is a version that works in 1.2.  The only change is that ".width" and ".height" on master change to ".w" and ".h" on v1.2.  I've tested on a 1.2 buri phone and it seems to work.

I'll land and uplift when the gaia tree reopens.

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/482e84753f32

Comment 93

[David Flanagan [:djf]]

Gaia patch landed to master: https://github.com/mozilla-b2g/gaia/commit/54c342cffb334a3cb3d5658ab9131d967fd7e831

Comment 94

[David Flanagan [:djf]]

The v1.2 patch had a Travis build failure. Normally travis displays a "restart" button so I can try again.  But this time there is no button (perhaps because the tree was closed?)

So until I figure that out, the patch still needs to be uplifted to the v1.2 branch.

Comment 95

[David Flanagan [:djf]]

Removing the [KEEP OPEN] from the whiteboard since I landed the gaia patch to master.  We can now close this when the gecko patch lands on m-c, I guess.

Comment 96

[David Flanagan [:djf]]

Attachment: gaia patch for the v1.2 branch

I've created a new version of the v1.2 branch pull request because there was something wrong with Travis on the old one and I couldn't restart test runs.

Comment 97

[David Flanagan [:djf]]

The new PR does not get Travis to run because travis builds are based on the commit, not on the PR... that should have been obvious to me.

Comment 98

[David Flanagan [:djf]]

Finally got a green Travis build for the v1.2 branch. 

Uplifted to v1.2: https://github.com/mozilla-b2g/gaia/commit/8d762f3376318fd6be390432db750ae4904c9ab6

Comment 99

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/482e84753f32

Comment 100

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c2a3a29c82fe

Comment 101

[David Teller [:Yoric] - still alive but not very active]

(In reply to David Flanagan [:djf] from comment #59)
> The 07/01 build does not crash or OOM. The file pick does not seem to
> complete successfully, however, and I see this in the console: 
> 
> E/GeckoConsole(  569): [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component
> returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE)
> [nsIMIMEService.getFromTypeAndExtension]" {file:
> "jar:file:///system/b2g/omni.ja!/components/FilePicker.js" line: 164}]
> 
> There is a 7/22 change to FilePicker.js. Next I should check whether the
> crash started when that change landed.

I have the same error on a Unagi running FirefoxOS 1.1, while picking a .cbz or .zip file.