1080104 - [Gallery] remove the memory-backed blob workaround hack from bug 1063658

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

In bug 1063658 we landed a workaround for bug 1079546. When 1079546 is fixed, we should remove the workaround.

Comment 1

[David Flanagan [:djf]]

Nominating this for 2.2 so it gets on a list and we don't forget to do it.

Comment 2

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

David, bug 1079546 has landed, so I think you should be able to remove your workarounds now.

Comment 3

[David Flanagan [:djf]]

Thanks Ben.

Comment 4

[David Flanagan [:djf]]

From bug 1079546, it looks like the gecko fix will fix the bug for v2.2 only, and the workaround will need to remain in place for v2.0 and v2.1.  So unless we hear otherwise in 1079546, this but should only revert the fix in 1063658 on master.

Comment 5

[David Flanagan [:djf]]

Attachment: patch to revert the workaround

Punam: would you review this please?

I had an old version of gecko still on my phone. I applied this patch and verified that the sms app crashed if I picked a cropped image from gallery.

Next, I updated my photo to today's nightly and applied this patch again. This time, I could successfully pick a cropped image without crashing the sms app. So it does look like the underlying gecko bug has been resolved.

I did not test with the email app which was the original focus for this workaround, however.

Comment 6

[David Flanagan [:djf]]

After discussing this with Punam, she is going to wait to review this patch until 1080090 is resolved, since that is another gallery-related regression caused by blob and indexeddb code. That way we can make sure that the fix for 1080090 does not cause problems with the fix for 1079546. We don't want to remove this workaround and then just have to land it again.

Comment 7

[Punam Dahiya [:pdahiya]]

Comment on attachment 8507088 [details] [review]
patch to revert the workaround

Hi David

I am not able to replicate gallery crash seen in bug1080090 in today's build.

Gaia      e91d99e4d96954f06383c00bb9d79598a697e310
Gecko     https://hg.mozilla.org/mozilla-central/rev/8230834302c9
BuildID   20141027040237
Version   36.0a1
device: Flame-KK v180 base image

We should revert this workaround in master. I have verified both SMS and email flow and able to attach image successfully after reverting the workaround with the attached patch. Thanks!

Comment 8

[David Flanagan [:djf]]

I don't remember why I never landed this patch after Punam reviewed it. The workaround is still in gallery and my patch is bit-rotted.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8507088 [details] [review]
patch to revert the workaround

Punam, would you review this again? I've updated the patch so it will apply.

Maybe before landing it we should land the pick activity test that you're going to be reviving from bug 959430.

Actually, if you've got the time, maybe you could take the pick activity test from bug 959430 and use in bug 1080122, and also write a share activity test for that bug. If we land those tests it should be totally safe to land this workaround removal. I filed bug 1080122 at the same time I filed this bug, with the thought that we should land tests before removing the workaround, and that still seems like a good idea if we can get it done.

Writing marionette tests for activities may actually be quite hard. This isn't something that is worth spending weeks on, but if you can get those tests written in a few days it would be really great to finally have them written!

Comment 10

[Punam Dahiya [:pdahiya]]

Hi David

We have activity tests landed on master in Bug 1080122. If you can rebase attached patch with latest master, it will run those tests on this PR on tree herder. 

Setting NI flag for you to help rebase this patch for activity tests to run on treeherder on this PR. Thanks!

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] punamdahiya:Bug1080104 > mozilla-b2g:master

Comment 12

[Punam Dahiya [:pdahiya]]

Comment on attachment 8507088 [details] [review]
patch to revert the workaround

Attached rebased patch attachment 8645931 [details] [review], will set review flag after treeherder job completes.

Comment 13

[Punam Dahiya [:pdahiya]]

Comment on attachment 8645931 [details] [review]
[gaia] punamdahiya:Bug1080104 > mozilla-b2g:master

Hi David

Attached patch is rebased and passing on treeherder. I am not able to set review flag on this patch. Setting NI flag to review and land the patch. Thanks!

Comment 14

[David Flanagan [:djf]]

landed: https://github.com/mozilla-b2g/gaia/commit/342f5b3f7865e68b1885258c53c23204f6da1b97