816322 - "Add Picture" from Gallery intermittently fails to transfer chosen image

Status: RESOLVED WORKSFORME

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

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Follow https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW2:_Active_call_stays_active_.5BPARTIAL-PASS.5D

Sometimes in step 27 and 28, the "Crop" screen doesn't show the selected image, and the Contacts app gets back a blank image.

This shouldn't fail.

Comment 1

[David Flanagan [:djf]]

Vivien reports this logcat when he sees this bug: 

    D/memalloc(  500): /dev/pmem: Unmapping buffer base:0x44bc5000 size:4669440 offset:4628480
    E/GeckoConsole(  500): Content JS ERROR at app://gallery.gaiamobile.org/shared/js/mediadb.js:556 in getFile/getRequest.onerror: MediaDB.getFile: SecurityError
    I/IdleService(  109): Get idle time: time since reset 930 msec
    I/IdleService(  109): Idle timer callback: current idle time 930 msec
    I/IdleService(  109): next timeout 1354232293650019 usec (69 msec from now)
    I/IdleService(  109): SetTimerExpiryIfBefore: next timeout 1354232293650019 usec
    I/IdleService(  109): reset timer expiry from 0 usec to 1354232293660019 usec
    I/IdleService(  109): Get idle time: time since reset 1009 msec
    I/IdleService(  109): Idle timer callback: current idle time 1009 msec
    I/IdleService(  109): next timeout 1354232472650019 usec (178990 msec from now)
    I/IdleService(  109): SetTimerExpiryIfBefore: next timeout 1354232472650019 usec
    I/IdleService(  109): reset timer expiry from 0 usec to 1354232472660019 usec
    I/IdleService(  109): Idle timer callback: tell observer 47a388a0 user is idle
    I/IdleService(  109): Get idle time: time since reset 1009 msec
    E/GeckoConsole(  500): [JavaScript Error: "RangeError: argument 1 accesses an index that is out of range" {file: "app://gallery.gaiamobile.org/shared/js/blobview.js" line: 92}]
    E/GeckoConsole(  526): [JavaScript Warning: "Unknown property 'align-self'.  Declaration dropped." {file: "resource://gre-resources/ua.css" line: 45}]
    E/GeckoConsole(  526): [JavaScript Warning: "Unknown property 'order'.  Declaration dropped." {file: "resource://gre-resources/ua.css" line: 46}]
    I/IdleService(  109): next timeout 1354232301951380 usec (999 msec from now)
    I/IdleService(  109): SetTimerExpiryIfBefore: next timeout 1354232301951380 usec
    I/IdleService(  109): reset timer expiry from 1354232472660019 usec to 1354232301961380 usec
    I/IdleService(  109): Reset idle timeout: tell observer 47a388a0 user is back
    E/GeckoConsole(  500): [JavaScript Error: "TypeError: cropEditor is undefined" {file: "app://gallery.gaiamobile.org/js/gallery.js" line: 706}]

Comment 2

[David Flanagan [:djf]]

Attachment: image that causes the bug for vivien

Comment 3

[Doug Turner (:dougt)]

bent, could be related to bug 806503?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Dunno... Does the image get saved to indexedDB before the blob is passed? I think that is djf's preferred workaround for this kind of thing...

Comment 5

[David Flanagan [:djf]]

Doug, Ben:

Thanks for looking at this. The report has the crop screen itself failing, before the blob is transferred back to the caller, so this wouldn't be related to 806503.

Doug, what do you make of the SecurityError on the second line of the logcat above?
That's being printed by the onerror callback of a DeviceStorage.get() request. What kind of security checks would be happening there? 

If gallery is invoked for a pick activity from contacts, it isn't running with contacts permissions, is it?

The way this would work is that gallery pulls a bunch of thumbnails out of indexeddb. Then when the user finally clicks on one, it goes to get the actual file from device storage.  The indexdb stuff is working, but the device storage is intermittently failing with this security error.

(I have not attempted to reproduce this myself yet).

Is there anyone that should be cc'ed on this because of the security stuff?

Comment 6

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 7

[Doug Turner (:dougt)]

SecurityError means that either the path to the file isn't "safe" or the permission wasn't granted to the application.

Since the file name has a ~ in it, it triggers the failure case of IsSafePath().  So, Device Storage will not be able to handle these file names right now.

My question is, how did you initially get thumbnails in indexeddb -- if Device Storage can't return files that have a tilde in them....

If supporting ~ in a file name is the only bug here, please reassign to me.

Comment 8

[David Flanagan [:djf]]

(In reply to Doug Turner (:dougt) from comment #7)
> SecurityError means that either the path to the file isn't "safe" or the
> permission wasn't granted to the application.

This bug occurs during activities so I suppose it is possible that it is a permissions thing rather than an unsafe file name.  Is tilde the only reason for unsafe filenames?  

> Since the file name has a ~ in it, it triggers the failure case of
> IsSafePath().  So, Device Storage will not be able to handle these file
> names right now.
> 
> My question is, how did you initially get thumbnails in indexeddb -- if
> Device Storage can't return files that have a tilde in them....
> 

Could it be that the DeviceStorage enumerate() and getFile() methods apply the safety check differently?  If there is a ~ in the base file name (i.e. not a path component, but part of the filename itself, like Windows does when converting long file names to 8.3 filenames) could the file be enumerated (and therefore inserted into the database) but not available via getFile()?

> If supporting ~ in a file name is the only bug here, please reassign to me.

I haven't tried to reproduce this bug yet. If I can do it with files that contain tildes then I'll reassign to you.

Comment 9

[David Flanagan [:djf]]

Doug,

You rock. Thanks for figuring out the tilde thing. I can reproduce this trivially on the desktop just by putting a photo with ~ in the filename into the ~/Pictures directory.  The file is found during scanning (via device storage enumeration) but I can't call getFile() on it.

So I'm guessing that enumerate and getFile apply different versions of the filename safety check, so I'm assigning to you.  I'd prefer to allow files like Vivien's that include an embedded tilde. But I'd live with a fix that just didn't enumerate them at all.

Chris: as the original reporter, is it possible that you had files with tildes in their names when you reported this bug?

Comment 10

[David Flanagan [:djf]]

Vivien: do you know how your cat picture ended up with a tilde in its name?  When you mount your phone as USB mass storage and copy files to it, do they end up having their names converted to uppercase MS-DOS 8.3 filenames with tildes?  

I don't see this happening on MacOS, but I wonder if there is something about the way Linux mounts the device that makes it think it is a partition that can not handle long file names.

Doug: if that is the case, then we really do need getFile() to support safe tildes and can't just have enumerate() skip them.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to David Flanagan [:djf] from comment #9)
> Chris: as the original reporter, is it possible that you had files with
> tildes in their names when you reported this bug?

Sadly no.

Comment 12

[David Flanagan [:djf]]

Chris,

It looks like we have isolated and Doug can fix the bug that was affecting Vivien. Vivien's symptoms sounded the same as yours.

Can you still reproduce what you were seeing? And if so, is there anything in the logcat output?

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm not able to complete https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW2:_Active_call_stays_active because the "Add Image" picker is broken.

Comment 14

[Tony Chung [:tchung]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> I'm not able to complete
> https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW2:
> _Active_call_stays_active because the "Add Image" picker is broken.

i was able to add images in contacts on the 12-17 daily unagi builds (build 20121217070202).  [both through gallery and camera share]

please renom if you can still reproduce.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I wasn't able to reproduce on today's first run because the b2g process crashed.

I tried again and reproduced.

Are you following the steps in the test case?

Comment 16

[Doug Turner (:dougt)]

There are two different problems here.  We are preventing ~'s in files names from being returned by device storage.  I'll fix that in a clone of this bug (coming up).

I'll leave this bug open for the crash that cjones is seeing.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(The crash I mentioned in comment 15 is another bug.)

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to David Flanagan [:djf] from comment #10)
> Vivien: do you know how your cat picture ended up with a tilde in its name? 
> When you mount your phone as USB mass storage and copy files to it, do they
> end up having their names converted to uppercase MS-DOS 8.3 filenames with
> tildes?  
> 

Likely my OS (Linux) has create it for me.

Comment 19

[David Scravaglieri [:scravag]]

There are already two issues for this bug

Comment 20

[Doug Turner (:dougt)]

david, what are the two other issues for this bug?  Why is it minused?

Comment 21

[David Flanagan [:djf]]

I can't reproduce the bug.  Chris still can. I wonder what the differences are.  

Chris: when you run this test, is it with a phone that you have done a reset-gaia on? If so, then you're getting the gallery on its first run, which means it has to scan the sdcard and process the images.  I don't think that should make a difference, but it might be relevant. When I just ran the test, the gallery db was not new.

Chris: have you tried with pictures other than the one listed in the testing script? In particular, can you reproduce it with a picture you take with the camera?

This is the first time I've actually see the green "on call" bar at the top of the display. Since that is bigger than the normal status bar, it means the app window is smaller.  Maybe something in the app has a dependency on a 460px screen height?  But if that was the case, it seems like it would be more consistently reproducible.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to David Flanagan [:djf] from comment #21)
> I can't reproduce the bug.  Chris still can. I wonder what the differences
> are.  

Just to be clear, you couldn't reproduce when following the STR?

> Chris: when you run this test, is it with a phone that you have done a
> reset-gaia on? If so, then you're getting the gallery on its first run,
> which means it has to scan the sdcard and process the images.  I don't think
> that should make a difference, but it might be relevant. When I just ran the
> test, the gallery db was not new.

No, after first scan.

> Chris: have you tried with pictures other than the one listed in the testing
> script? In particular, can you reproduce it with a picture you take with the
> camera?

Yes, but I don't recall trying with an image captured from the camera.

However, all that said, I'm not able to reproduce today for the first time.  Yay? :/

Comment 23

[David Flanagan [:djf]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> (In reply to David Flanagan [:djf] from comment #21)
> > I can't reproduce the bug.  Chris still can. I wonder what the differences
> > are.  
> 
> Just to be clear, you couldn't reproduce when following the STR?
> 

Yes, I finally ran through the long STR and could not reproduce.

> However, all that said, I'm not able to reproduce today for the first time. 
> Yay? :/

Yay!