bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 28, Firefox OS v1.2

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Matthew Vaughan, Assigned: djf)

Tracking

({regression})

unspecified
mozilla28
ARM
Gonk (Firefox OS)
regression
Points:
---

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed)

Details

(Whiteboard: burirun3 [MemShrink:P1][systemsfe], burirun4)

Attachments

(11 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 827681 [details]
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
(Reporter)

Comment 1

5 years ago
Created attachment 827683 [details]
LogCat of attempting to upload an image to Facebook.
(Reporter)

Comment 2

5 years ago
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
status-b2g18: --- → unaffected
status-b2g-v1.2: --- → affected
Keywords: regression

Updated

5 years ago
blocking-b2g: --- → koi?
jwatt/baku, any thoughts here?  Could this be related to file input types?
blocking-b2g: koi? → koi+
Component: Gaia::Browser → DOM
Flags: needinfo?(jwatt)
Flags: needinfo?(amarchesini)
Product: Firefox OS → Core
wrong bug... sorry.
Btw: No ideas yet. I'll try to reproduce it.
FWIW I don't know of anything that would cause this either. Hopefully baku can repro.
Flags: needinfo?(jwatt)

Comment 7

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

Updated

5 years ago
Whiteboard: burirun3 → burirun3 [MemShrink]

Comment 8

5 years ago
(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!)
(Please re-add the Memshrink tag once you're more certain this is OOM related :)
Whiteboard: burirun3 [MemShrink] → burirun3
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.
Keywords: qawanted
(Reporter)

Updated

5 years ago
QA Contact: mvaughan
(Reporter)

Comment 11

5 years ago
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
Keywords: qawanted
(Reporter)

Comment 12

5 years ago
Created attachment 831804 [details]
Facebook dmesg
(Reporter)

Comment 13

5 years ago
Created attachment 831806 [details]
Flickr dmesg
(Reporter)

Comment 14

5 years ago
Created attachment 831807 [details]
Tumblr dmesg

Updated

5 years ago
Whiteboard: burirun3 → burirun3 [MemShrink]
NI on :overholt to help with assignee here.

Updated

5 years ago
Flags: needinfo?(overholt)
Milan,

Please check if this is gallery related?
Flags: needinfo?(milan)
See bug 921659 for general issue about memory usage regression. That's likely to play a role here.
Gregor suggested a memory profile could help track this down.  Mike, can someone on the perf team help with that?
Flags: needinfo?(overholt) → needinfo?(mlee)
(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.
Flags: needinfo?(milan)

Comment 20

5 years ago
Hema,

Please assign someone on the Gallery team to look into this. Debugging instructions are available here: https://wiki.mozilla.org/B2G/Debugging_OOMs
Flags: needinfo?(mlee) → needinfo?(hkoka)

Updated

5 years ago
Keywords: regressionwindow-wanted
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.
Gregor,

Can you please check if this bug is related to browser? Please re-assign appropriately.
Flags: needinfo?(anygregor)
(Assignee)

Comment 23

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → dflanagan
Flags: needinfo?(hkoka)
Dale, can you help out here?
Flags: needinfo?(anygregor)
Whiteboard: burirun3 [MemShrink] → burirun3 [MemShrink][systemsfe]
(Reporter)

Comment 25

5 years ago
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
Keywords: regressionwindow-wanted
Can you go deeper with that regression range by trying different workflows to ensure a crash doesn't happen?
Keywords: regressionwindow-wanted
(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..
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?
(Reporter)

Comment 29

5 years ago
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
Keywords: regressionwindow-wanted
(Reporter)

Comment 30

5 years ago
Created attachment 8334658 [details]
Dmesg log from the 11/19/13 1.2 build
(Assignee)

Comment 31

5 years ago
(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.
Flags: needinfo?(dale)
Flags: needinfo?(bfrancis)
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
Assignee: dflanagan → dale
Flags: needinfo?(dale)
Created attachment 8336004 [details] [diff] [review]
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.
Attachment #8336004 - Flags: feedback?(dale)
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
Attachment #8336004 - Flags: feedback?(dale) → feedback+
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
Flags: needinfo?(dflanagan)
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
What Dale said.
Flags: needinfo?(bfrancis)
Hmm, patch doesnt stop the tab from refreshing, thinking that this isnt an oom killer issue 

http://pastebin.mozilla.org/3647549
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.
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
Assignee: dale → dflanagan
(Assignee)

Comment 41

5 years ago
(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.
Flags: needinfo?(lissyx+mozillians)
(Assignee)

Comment 42

5 years ago
(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.
Flags: needinfo?(dflanagan)
(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 :)
Flags: needinfo?(lissyx+mozillians)
Whiteboard: burirun3 [MemShrink][systemsfe] → burirun3 [MemShrink][systemsfe], burirun4
(Assignee)

Comment 44

5 years ago
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.
(Assignee)

Comment 45

5 years ago
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
Flags: needinfo?(mvaughan)
Flags: needinfo?(milan)
Keywords: regressionwindow-wanted
(Assignee)

Comment 46

5 years ago
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.
(Reporter)

Comment 47

5 years ago
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?
Flags: needinfo?(mvaughan)
Keywords: regressionwindow-wanted
(Assignee)

Comment 48

5 years ago
Created attachment 8337139 [details] [review]
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.
(Assignee)

Comment 49

5 years ago
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?)
Flags: needinfo?(mvaughan)
(Assignee)

Comment 50

5 years ago
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?
(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.
Flags: needinfo?(milan)
(Assignee)

Comment 52

5 years ago
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.
(Assignee)

Comment 53

5 years ago
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.
(Assignee)

Comment 54

5 years ago
Created attachment 8338287 [details]
memory-reports
(Assignee)

Comment 55

5 years ago
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.
(Assignee)

Comment 56

5 years ago
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.
(Assignee)

Comment 57

5 years ago
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.
(Assignee)

Comment 58

5 years ago
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.
(Assignee)

Comment 59

5 years ago
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.
(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?
(Assignee)

Comment 61

5 years ago
(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 :-)
(Assignee)

Comment 62

5 years ago
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.
(Assignee)

Comment 63

5 years ago
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.
(Assignee)

Comment 64

5 years ago
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.
Flags: needinfo?(timdream)
Flags: needinfo?(alive)
(Assignee)

Comment 65

5 years ago
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.
Attachment #8337139 - Flags: review?(johu)
> 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.
Let me know if the MemShrink team can help here.
Whiteboard: burirun3 [MemShrink][systemsfe], burirun4 → burirun3 [MemShrink:P1][systemsfe], burirun4
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.
Flags: needinfo?(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?
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.
Flags: needinfo?(timdream)
(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

5 years ago
Based on Alive's comment 68, adding Fabrice for input.
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
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.
Attachment #8337139 - Flags: review?(johu) → review-
(Assignee)

Updated

5 years ago
Attachment #8336004 - Flags: review?(fabrice)
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'] = ...
Attachment #8336004 - Flags: review?(fabrice) → review+
(Assignee)

Comment 76

5 years ago
Andrea,

Would you update your patch to address Fabrice's review nit, and then land it (or set checkin-needed?)
Flags: needinfo?(amarchesini)
Created attachment 8341311 [details] [diff] [review]
crop.patch
Attachment #8336004 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Attachment #8337139 - Flags: review- → review?(johu)
(Assignee)

Comment 78

5 years ago
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 on attachment 8337139 [details] [review]
link to patch on github

Nice patch. It works. r=me.
Attachment #8337139 - Flags: review?(johu) → review+
https://hg.mozilla.org/mozilla-central/rev/ed9645b3d6ec
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Assignee)

Comment 82

5 years ago
Reopening, since the Gaia patch still needs to land
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 83

5 years ago
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?
Flags: needinfo?(amarchesini)
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.
status-b2g-v1.1hd: --- → unaffected
status-firefox26: --- → wontfix
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
Flags: needinfo?(mvaughan)
If the PR is ready to merge, please put checkin-needed on the bug.
(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?
Flags: needinfo?(amarchesini)
Created attachment 8341775 [details] [diff] [review]
filterAll.patch

This is the patch that adds nocrop if no filters are set.
Attachment #8341775 - Flags: review?(dflanagan)
(Assignee)

Comment 88

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

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

A simple followup to the previous patch.
Attachment #8341775 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 89

5 years ago
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.
Keywords: checkin-needed
Whiteboard: burirun3 [MemShrink:P1][systemsfe], burirun4 → burirun3 [MemShrink:P1][systemsfe], burirun4 [KEEP OPEN]
(Assignee)

Comment 90

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

Comment 91

5 years ago
Created attachment 8341919 [details] [review]
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.
(Assignee)

Comment 94

5 years ago
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.
(Assignee)

Comment 95

5 years ago
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.
Whiteboard: burirun3 [MemShrink:P1][systemsfe], burirun4 [KEEP OPEN] → burirun3 [MemShrink:P1][systemsfe], burirun4
(Assignee)

Comment 96

5 years ago
Created attachment 8342197 [details] [review]
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.
Attachment #8341919 - Attachment is obsolete: true
(Assignee)

Comment 97

5 years ago
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.
(Assignee)

Comment 98

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

Uplifted to v1.2: https://github.com/mozilla-b2g/gaia/commit/8d762f3376318fd6be390432db750ae4904c9ab6
https://hg.mozilla.org/mozilla-central/rev/482e84753f32
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 946519

Updated

5 years ago
No longer depends on: 946519
(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.
You need to log in before you can comment on or make changes to this bug.