Closed Bug 1076975 Opened 10 years ago Closed 10 years ago

[Ringtones] Songs are not added to ringtones list

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.2+, firefox35+ verified, firefox36 verified, b2g-v2.0 unaffected, b2g-v2.1 unaffected, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S7 (24Oct)
blocking-b2g 2.2+
Tracking Status
firefox35 + verified
firefox36 --- verified
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified

People

(Reporter: AdamA, Assigned: bent.mozilla)

References

()

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

Description:
When a user tries to add a song as a ringtone it will never appear in the list of ringtones as an option. This occurs wether the user adds the song through settings or the music app.
   
Repro Steps:
1) Update a Flame device to BuildID: 20141001170320
2) Go to Setting > Sounds > Manage Tones
3) Click the "+" icon in the top right
4) Select a song and add it as a ringtone.
5) Observe list of ringtones options
  
Actual:
The song is never added as a ringtone option.
  
Expected: 
It is expected that the song is added as a ringtone option
  
Environmental Variables:
Device: Flame 2.2 Master (Shallow Flash)
BuildID: 20141001170320
Gaia: 191d805f4911628d37a8a90a1e23a6013995138f
Gecko: 2399d1ae89e9
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Repro frequency: 100%
See attached: video clip(http://youtu.be/4LO-1PQvCBM), logcat
This issue does not occur in 2.1 Flame or 2.0 Flame.

Environmental Variables:
Device: Flame 2.1 (Shallow Flash)
BuildID: 20141001123325
Gaia: c1cc61e30f5cb3446f1692ff9fd1e32232f6a231
Gecko: 4ff49aa32213
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.0 (Shallow Flash)
BuildID: 20141001123531
Gaia: 8079cba2133e6f5443dba24dad077f7f91e6c978
Gecko: 3a656e533675
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Result:
Songs are added to ringtones
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: regression
Regression window, please! :)
QA Contact: aalldredge
As I was working on this issue I noticed that in the builds soon after it was broken there was an error message that appeared. The error message that appears informs the user that there was a problem creating the ringtone. Attached a screenshot of the error message. I have never seen this error in later builds that the issue is occurring in.

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20140929064600
Gaia: 2834baf4c7e34fe6ef335f0469f6d0f593c5922b
Gecko: f57209ddc739
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
Nomming for 2.2, regression, broken functionality
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
----------------------------------------------------
Mozilla-Inbound Regression Window (Shallow Flash)
----------------------------------------------------

Last Working:
Device: Flame 2.2 Master
BuildID: 20140926154139
Gaia: e30d373eb5ed0514bce08a3b3d80d71b05a4dc32
Gecko: 627e848b2bf3
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

First Broken:
Device: Flame 2.2 Master
BuildID: 20140926162338
Gaia: e30d373eb5ed0514bce08a3b3d80d71b05a4dc32
Gecko: 8892214038df
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Last Working Gaia First Broken Gecko: Issue DOES reproduce
Gaia: e30d373eb5ed0514bce08a3b3d80d71b05a4dc32
Gecko: 8892214038df

First Broken Gaia Last Working Gecko: Issue does NOT reproduce
Gaia: e30d373eb5ed0514bce08a3b3d80d71b05a4dc32
Gecko: 627e848b2bf3

Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=627e848b2bf3&tochange=8892214038df

Caused by Bug 994190
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Caused by Bug 994190 ? Can you take a look Ben?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(bent.mozilla)
Hi! I've been taking a look at this bug and it seems to be a performance problem. At first, I set up my flame with 273MB and I got this result when trying to save the ringtone without check the "Set default ringtone":

W/Ringtones( 3802): [JavaScript Error: "QuotaExceededError" {file: "app://ringtones.gaiamobile.org/js/custom_ringtones.js" line: 260}]
E/Ringtones( 3802): Content JS ERROR: Error in customRingtones.add():  QuotaExceededError

In this case, the error is shown to the user.

If the checkbox is checked the output is this:

I/Gecko   (  207): ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x120005,name=PBackground::Msg_PBlobConstructor) Value error: message was deserialized, but contained an illegal value
I/Gecko   (  207): 
I/Gecko   (  207): IPDL protocol error: could not look up PBlob
I/Gecko   (  207): IPDL protocol error: Error deserializing 'PBlobParent'
I/Gecko   (  207): 
I/Gecko   (  207): ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x60004,name=PBackgroundIDBDatabase::Msg_PBackgroundIDBDatabaseFileConstructor) Value error: message was deserialized, but contained an illegal value
I/Gecko   (  207): 
I/Gecko   (  207): IPDL protocol error: could not look up PBackgroundIDBDatabaseFile
I/Gecko   (  207): IPDL protocol error: Error deserializing 'DatabaseFileOrMutableFileId[i]'
I/Gecko   (  207): IPDL protocol error: Error deserializing 'files' (DatabaseFileOrMutableFileId[]) member of 'ObjectStoreAddPutParams'
I/Gecko   (  207): IPDL protocol error: Error deserializing 'commonParams' (ObjectStoreAddPutParams) member of 'ObjectStoreAddParams'
I/Gecko   (  207): IPDL protocol error: Error deserializing 'RequestParams'
I/Gecko   (  207): 
I/Gecko   (  207): ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0xE0006,name=PBackgroundIDBTransaction::Msg_PBackgroundIDBRequestConstructor) Value error: message was deserialized, but contained an illegal value

Hope this will be helpful
Thanks!
Adam: now that bug 994190 has been resolved, can you still reproduce this bug?

Jim: is this bug on your radar?

Manuel: those are horrible error messages in your logcat. What versions of gecko and gaia are you testing with?  (Note also that we no longer test the flame at 273. I think all our testing now uses a minimum of 319mb on Flame.)
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(b.mcb)
Flags: needinfo?(aalldredge)
This issue is still occurring in 2.2 Flame Master.

Environmental Variables:
Device: Flame 2.2 Master (Full Flash)
BuildID: 20141007040205
Gaia: 83de447d9ae9a59459d7a445f9348a254c661850
Gecko: 9ee9e193fc48
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
Flags: needinfo?(aalldredge)
I'm aware of this bug. Also, I think the point was that bug 994190 is what regressed us. It also looks like it broke email (specifically, ActiveSync, so I guess someone out there really likes breaking apps I worked on!).
Flags: needinfo?(squibblyflabbetydoo)
Attached file logcat
Device: Flame 2.2 Master 319mb
BuildID: 20141007060002
Gaia: 4c5730799ed0aa1c992325401ce063ecd2d81e5b
Version: 35.0a1
Firmware: V184

See the complete log attached.
Flags: needinfo?(b.mcb)
If the music app is returning a memory-backed blob for some reason instead of a file-backed blob, then we might have problems like those in bugs 1063658 and 1077106.  Is the ringtone app doing metadata parsing on the blob it receives? If so, then it is probably calling slice() on that remote memory-backed blob which seems to be the root issue.  I'm landing a gallery workaround in bug 1063658. If the issue is similar in the music app, then a similar workaround could probably work.

Jim: could memory-backed blobs be the cause here?

Adam: do you see any similar bugs if you use the share activity in the music app to set a ringtone?
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(aalldredge)
The ringtones app doesn't parse metadata anywhere, and indeed, we never call slice() at all in the ringtones app.
Flags: needinfo?(squibblyflabbetydoo)
bent is working on undoing all the regressions caused by bug 994190, so I think if we just wait a week, this bug will go away.
When I set a song as a ringtone from the music app I see the same behaviour. The ringtone is not created.
Flags: needinfo?(aalldredge)
Ben: Do you know if there's a bug already filed that would fix this issue? We'd like to track the progress of it.
Can you please test again now that bug 1079546 has landed?
Flags: needinfo?(bent.mozilla)
I just reflashed a new Gaia/Gecko with the changes from bug 1079546, and the problem still happens.
Jim, can you attach logcat for your problem? (Or, is it the same as one posted here already?)
Flags: needinfo?(squibblyflabbetydoo)
Here's my logcat. It seems to be a bit different from the others (but the 2nd logcat on this bug is a totally different issue, I think).
Flags: needinfo?(squibblyflabbetydoo)
Attached patch Patch, v1 (obsolete) — Splinter Review
This patch reworks serialization so that we don't mess with input streams any more (except for FileHandle snapshots...).
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #8507642 - Flags: review?(khuey)
Attached patch Patch, v1.1Splinter Review
Attachment #8507642 - Attachment is obsolete: true
Attachment #8507642 - Flags: review?(khuey)
Attachment #8508244 - Flags: review?(khuey)
Comment on attachment 8508244 [details] [diff] [review]
Patch, v1.1

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

::: dom/ipc/DOMTypes.ipdlh
@@ +56,5 @@
>    nsString contentType;
>    uint64_t length;
> +
> +  // This must be of type BlobData in a child->parent message, and will always
> +  // be of type void_t in a parent->child message.

Would be nice if IPDL had semantics to enforce this at the protocol level.

@@ +91,5 @@
>  {
>    nsID id;
>  };
>  
> +// XXX This may only be used for same-process inter-thread communication! The

NB, not XXX.
Attachment #8508244 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/33f348ff0417
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Hi Hema, this feature apparently broke even though TBPL remained completely green. Can you find resources to get integration tests added for this so that we don't break you again?
Flags: needinfo?(hkoka)
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #28)
> Hi Hema, this feature apparently broke even though TBPL remained completely
> green. Can you find resources to get integration tests added for this so
> that we don't break you again?

We actually have integration tests for this, but they didn't fail when your patch landed. I'm guessing that this never actually broke on desktop builds, but did break on devices.
Blocks: 1087464
blocking-b2g: 2.2? → 2.2+
Target Milestone: --- → 2.1 S7 (24Oct)
See comment 29
Flags: needinfo?(hkoka)
This issue is verified fixed on Flame 2.2

Results: New song is added as ringtone option

Flame 2.2
Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141121040204
Gaia: 25388c6bce932657ebf93adedf31881bfaf88c15
Gecko: 3366c0fcf9c2
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
[Tracking Requested - why for this release]:
Storing a File from input[type="file"] into IndexedDB is currently broken in Fx 35 Beta, but works fine in Fx 36 DevEdition. I've verified the nightly fix range is 20141021030208 - 20141022030202.

IIUC, for desktop Firefox, this bug was introduced in Fx 35 and fixed in Fx 36, I think this fix should be uplifted for Fx 35.
ni to get more attention, maybe I should request beta approval next time?
Flags: needinfo?(lsblakk)
This is fixed on 36 which I believe is the next base for FxOS, not 35 and so since we're about to build our RC for 35 I'm not going to take this extra churn when it has no impact to desktop users.
Flags: needinfo?(lsblakk)
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #36)
> This is fixed on 36 which I believe is the next base for FxOS, not 35 and so
> since we're about to build our RC for 35 I'm not going to take this extra
> churn when it has no impact to desktop users.

This bug is actually an IndexedDB issue, which is not specific to FxOS and affects desktop users as well. I'll provide a minimal test page for you to verify this soon.
(In reply to Hector Zhao [:hectorz] from comment #37)
> 
> This bug is actually an IndexedDB issue, which is not specific to FxOS and
> affects desktop users as well. I'll provide a minimal test page for you to
> verify this soon.

http://people.mozilla.org/~bzhao/file-into-indexeddb.html
Works in Fx 34/36 for desktop, not in Fx 35.
(In reply to Hector Zhao [:hectorz] from comment #38)
> http://people.mozilla.org/~bzhao/file-into-indexeddb.html
> Works in Fx 34/36 for desktop, not in Fx 35.

I'm 90% sure you can't be seeing this failure unless you're running with e10s enabled... Are you?
Flags: needinfo?(bzhao)
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #39)
> 
> I'm 90% sure you can't be seeing this failure unless you're running with
> e10s enabled... Are you?

No, I'm not.
Flags: needinfo?(bzhao)
[Tracking Requested - why for this release]:

Hector is right, this somehow slipped through our testing and tracking. I might be able to come up with a smaller patch here but I kinda doubt it. Given that there was only one not-really-related followup here (bug 1087464 - already fixed on 35) it might be worth taking...
Follow up from conversation - if this was to go into an RC build with very little time to test with users what is the risk?  Do we have tests that now catch the issue that was reported and do we have coverage that we can trust on this fix?
Flags: needinfo?(bent.mozilla)
Attached patch Test (obsolete) — Splinter Review
This test passes with the patch, fails without it.
Attachment #8544851 - Flags: review?(khuey)
Please nominate this for approval to both beta & release branch uplift since we're past merge, would like to have this landed tomorrow morning for a build 2 of our RC.
Comment on attachment 8508244 [details] [diff] [review]
Patch, v1.1

Approval Request Comment
[Feature/regressing bug #]: 994190
[User impact if declined]: <input type="file"> blobs can't be stored in indexedDB
[Describe test coverage new/current, TBPL]: New test here
[Risks and why]: This regression only affects 35. 34/36 are unaffected, so this would be a six-week regression. Nothing else has landed on 36 (I don't think) and no one has reported further problems with that branch so this should make the 35 and 36 branches equivalent
[String/UUID change made/needed]: None
Flags: needinfo?(bent.mozilla)
Attachment #8508244 - Flags: approval-mozilla-release?
Attachment #8508244 - Flags: approval-mozilla-beta?
Attachment #8508244 - Flags: approval-mozilla-release?
Attachment #8508244 - Flags: approval-mozilla-release+
Attachment #8508244 - Flags: approval-mozilla-beta?
Attachment #8508244 - Flags: approval-mozilla-beta+
Thanks for getting this fixed for Fx 35!
Reproduced the initial issue with the minimal test page from comment 38, on Windows 7 x64, using Firefox 35 RC build 1. In this case only "Pending" was displayed as the image was not being saved with success.

The issue no longer reproduces with Firefox 35 RC build 2 (BuildID=20150106233618) and the latest Firefox 36 Aurora (BuildID=20150107004006), on Windows 7 x64, Mac OS X 10.9.5, Ubuntu 12.04 x86. Now the "Success" message displays shortly after selecting various picture files.
So we're now permafailing the new test on beta and release across various platforms?
Flags: needinfo?(bent.mozilla)
Flags: in-testsuite+
Both beta and release show the same Android reftest failure starting with this patch landing as well:
https://treeherder.mozilla.org/logviewer.html#?job_id=205535&repo=mozilla-beta
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #50)

Hrm, this is a problem with the new test. There's a race in it...
Flags: needinfo?(bent.mozilla)
I pushed the changes to the test in bug 1118504 to m-i. Assuming it goes green I'll update the test on m-b and m-r.
How did you notice this issue?
Flags: needinfo?(bzhao)
Comment on attachment 8544851 [details] [diff] [review]
Test

Ok, so the new test has problems on Android and e10s due the way SpecialPowers works on those platforms. Since this bug originally landed with its own test (and it's passing everywhere already) I don't think we need to worry about getting this new test to run on m-b and m-r. Let's just deal with the test in bug 1118504.

I'll remove the test from m-b and m-r.
Attachment #8544851 - Attachment is obsolete: true
No longer depends on: 1118504
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #54)
> How did you notice this issue?

The alternative newtab page distributed by Beijing office has a feature, where users can upload an image to customize its background. Unfortunately we didn't find it out earlier, as we switched from __exposedProps__ to WebChannel to make our extension compatible with Fx 35 only very recently.
Flags: needinfo?(bzhao)
Thanks for pushing here Hector!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: