1002308 - [Sora][3rd][Notes+] MS will crash when enter camera and take a photo in Notes.

Status: RESOLVED FIXED

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

Description

[sync-1]

Mozilla build ID:20140404164003
 
 DEFECT DESCRIPTION:
  MS will carsh when enter camera and take a photo in Notes. 
 
  REPRODUCING PROCEDURES:
  1. Enter "Notes";
  2. Add a new Note;
  3. Select a photo from camera;
  4. Notes just crash. 
 
  EXPECTED BEHAVIOUR:
  Notes don't crash.
 
  CONTACT： 
  021-61460666-7531.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[Lingling Zhao]

The Notes+ displays as Notes in English(US), so in this PR Notes refers to Notes+.

1. I pre-installed Notes+ on V1.1 device, the issue does not happen.
2. I enter SMS, and add a photo from camera, click "Select", the issue does not happen.

Comment 2

[Lingling Zhao]

The Notes+ displays as Notes in English(US), so in this PR Notes refers to Notes+.

Comment 3

[Lingling Zhao]

Hi Vance,

Can you send it to someone to analyse it?

Thanks.

Comment 4

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

ni Dylan 

Hi Dylan, could you help to find someone to check this bug?

Thanks

Comment 5

[Dylan Oliver [:doliver]]

Vance -- I'm unable to reproduce on Flame or Peak (both running FxOS v1.3). 

Adding pictures from Camera or from Gallery are both working for me on the latest version of Notes+ (v2.2) from the Marketplace.

Comment 6

[Lingling Zhao]

Hi Vance,

This issue cannot be reproduced without the patch we got from bug 949941.

So it is bug 949941 broke this issue.

Please help to check.

Thanks.

Comment 7

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Dylan Oliver [:doliver] from comment #5)
> Vance -- I'm unable to reproduce on Flame or Peak (both running FxOS v1.3). 
> 
> Adding pictures from Camera or from Gallery are both working for me on the
> latest version of Notes+ (v2.2) from the Marketplace.

Hi Dylan -

Looks like our partner cheery-pick the fix of Bug#949941 from 1.3T branch into their code and hence has this problem. Do you have Taroko? Maybe you can try to reproduce this one with Tarako? 

Thanks

Vance

Comment 8

[Dylan Oliver [:doliver]]

Andrew, any thoughts on what might be happening here and/or is there anything Notes should be doing differently with respect to the Camera activity post- bug 949941? I'm able to reproduce the crash on a Peak running 1.4, but not on master.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

This should no longer reproduce on v1.4 as of today.  Bug 982779 has addressed the underlying system problem relating to Blobs and was uplifted to v1.4 (https://bugzilla.mozilla.org/show_bug.cgi?id=982779#c25).

I don't think an uplift of bug 982779 is in the cards for the v1.3 branches, but we hadn't really thought we needed it for v1.3 because the timing on v1.3 Buri and Tarako devices was such that it never posed a problem.  Faster or dual-core v1.3 devices may encounter sadness.

I'll take a look at the notes code a little to better understand what's going on and what the workarounds may be.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Okay, so the problem is that the Notes app is asking for a cropped copy of the image at
https://github.com/mozilla-b2g/notes/blob/master/js/common.js#L1649

This is likely resulting in a memory-backed Blob being provided which runs afoul of the platform issues from bug 982779.

There are a wide variety of options available of differing levels of quality:

- Best fix: See if bug 982779 can be taken on the v1.3 gecko 28 branch.  I just cherry-picked the patch and there seemed to only be one very small hunk that failed to apply.  On bug 982779 comment 26 I've asked whether this is something that would be safe to take.

- Second-best fix, cherry-pick the v1.3 workaround for the specific gallery problem.  Bug 975599 implemented a workaround so that in the cropping case a file-backed Blob would be provided rather than a memory-backed Blob.

- Bloat usage fix: have the notes app not request a specifically sized image in its activity.

- Have the notes app do the resizing workaround fix.  Steal the code from gallery to resize the image.  Run it in the notes app instead of in the gallery app.

Comment 11

[Lingling Zhao]

Hi Andrew,

Do you have a final result about how to fix it?

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

I am still waiting on the needinfo I referenced in comment 10.  I'll re-ping :bent in person tomorrow.  Leaving needinfo open on me for now to get that confirmation.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Lingling Zhao from comment #11)
> Hi Andrew,
> 
> Do you have a final result about how to fix it?

I just talked to :bent in person.  We don't think we can uplift bug 982779 to the v1.3 branch.  It looks like the workaround from bug 975599 should already be on the v1.3 branch per https://bugzilla.mozilla.org/show_bug.cgi?id=975599#c71.  Do you have that patch in your tree?

Comment 14

[Lingling Zhao]

Andrew,

The patch in bug 975599 has already in our tree.

We merge the patch in bug 949941 that broke this issue because there was a similar issue happened in Gmail, and this patch can fix the issue.

Comment 15

[Lingling Zhao]

Andrew, do you have any solution now?

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

The likely fix is to prepare a patch for your branch that backs out the part of bug 949941 that re-wraps a scaled Blob in a File wrapper to cause it to retain the original filename.

That hunk is here:
https://github.com/mozilla-b2g/gaia/commit/99890f3d9309ed5c696e110533aea5b6281f65d6#diff-4735c7e669157b8affda0f22473b3dc1R29

The change that needs to be made is removing these lines from apps/camera/js/lib/resize-image.js:

    // If the original blob was a File, propagate the name to a new File.  Note
    // that the file is still going to be memory-backed.  (Although technically
    // the Canvas implementation may opt to use a disk-backed cache.)
    if (originalBlob.name) {
      resizedBlob = new File([resizedBlob], originalBlob.name,
                             { type: resizedBlob.type });
    }

If one is feeling more thorough, the function blobResized can be removed in its entirety and the place where it is passed to toBlob changed to instead call 'done' directly.


I just attempted to create a patch for this but the v1.3t branch has apparently undergone more changes to this code , so without being able to look at the tree you are using, I unable to directly provide a patch.  If there's a publicly accessible tree I can formulate a patch or you should be able to make the changes on your own given the above.  (Specifically, the logic was moved into GAIA/shared/js/media/crop_resize_rotate.js, and it no longer seems to try and wrap the Blob in a File.  So if you had that code, I think your problem would already be fixed.)

Comment 17

[Lingling Zhao]

Hi Andrew,

After I remove these lines in our tree apps/camera/camera.js, both this issue and the issue in Gmail are not reproduced. 

Thanks for your help.

Comment 18

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

Can this bug be closed now then?

Comment 19

[Lingling Zhao]

Please close it, thanks for all your help.