1037079 - [B2G][Clock]Alarms do not go off when triggered. Timer alerts are also affected

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Ross Kuhlman ([e-mail])]

Attachment: ClockAlarmLog.txt

The Clock app Alarm and Timer are not functioning properly. Alarms do not go off when the set time is reached, and the Timer stops when the clock reaches 0, but otherwise does nothing. The clock continues to run, and the user can add minutes to the timer after it has reached zero. Pause/Resume also continues to function after the Timer reaches 0

Repro Steps:
1) Update a Flame to BuildID: 20140710071928
2) Launch Clock App
3) Set an alarm
4) Set a Timer

Actual:
Alarm does not go off when designated time is reached. No alert when Timer reaches zero. Timer buttons are still functional after clock reaches zero.

Expected:
Alarm goes off at designated time. Alert is displayed when Timer reaches zero.

Master M/C Environmental Variables:
Device: Flame Master M/C
BuildID: 20140710071928
Gaia: 09642e74e250fbc62db860c808ef188628fca55d
Gecko: f93c0ef45597
Version: 33.0a1
Firmware Version: v122

Notes:


Repro frequency: 100%
See attached: logcat

Comment 1

[Ross Kuhlman ([e-mail])]

This issue did not occur during v2.0 smoketest. Adding regression and regression-window-wanted tags.

Device: Flame 2.0
Build ID: 20140710000201
Gaia: 35a9b715e7348ec738ff6c8a59f50190390a06f2
Gecko: 94714370dfc3
Version: 32.0a2 (2.0)
Firmware Version: v122

Comment 2

[Peter Bylenga [:PBylenga]]

nominating for 2.1, is a regression of a core function with high visibility.

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

Mozilla inbound regression window:

Last Working Environmental Variables:
Device: Flame
Build ID: 20140709024902
Gaia: dde24450450514039bad6d8ab4fcb7e5d4d44e03
Gecko: 4bb01d74359a
Version: 33.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

First Broken Environmental Variable:
Device: Flame
BuildID: 20140709030031
Gaia: dde24450450514039bad6d8ab4fcb7e5d4d44e03
Gecko: ef24cd472cfb
Version: 33.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Gaia is the same on both builds. So it is caused by Gecko.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4bb01d74359a&tochange=ef24cd472cfb

Might have been caused by Bug 1015540? But I don't have permission to access that bug so I can't look into the bug further.

Comment 4

[Marcus Cavanaugh [:mcav]]

Based on that regression window, I agree that it looks like bug 1015540, but I don't have permission to access that either. It appears mozAlarms are completely borked with that patch.

Comment 5

[Joshua Mitchell (Inactive)]

Jason - none of us have permission to view the suspected bug so I don't know who to contact to take a look.

Comment 6

[Marcia Knous [:marcia]]

I added both of you to that bug.

Comment 7

[Joshua Mitchell (Inactive)]

(In reply to Marcia Knous [:marcia - use needinfo] from comment #6)
> I added both of you to that bug.

Thank you!



Sean - looks like your patch is the suspect for this bug, can you take a look please?

Comment 8

[Joshua Mitchell (Inactive)]

see also: bug 1037101, possible dupe

Comment 9

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v1

Fixing alarms and timer alerts.

Most changes in AlarmsManager.js are rolled back to what before the patch for bug 1015540. (https://hg.mozilla.org/mozilla-central/rev/cf143afd3029) But I still keep the sandbox when adding an alarm to ensure no cross-origin object can be involved. If the dry-run in the sandbox is passed, the raw data is used in the follow-up IPC calls without breaking alarm triggering behaviors.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8454292 [details] [diff] [review]
Patch v1

Please explain to me why this is safe.  When writing the explanation, please keep in mind that JSON.stringify() is not idempotent: two consecutive calls to it for the same object can return different results.  I expect that this explanation, to actually be convincing, will need to describe exactly what this._cpmm.sendAsyncMessage ends up doing with the "data" member in the dictionary it's passed.

As far as _I_ can tell sendAsyncMessage lands us in nsFrameMessageManager::DispatchAsyncMessage which calls GetParamsForMessage.  GetParamsForMessage tries to structured clone the input, and if that fails tries to structured clone JSON.parse(JSON.stringify(input)).  And during that JSON.stringify it would be running with the system principal and hence be able to extract privileged information from the input.  That says to me that this patch is _not_ safe.  What am I missing?

I would also like to see an explanation of why the code as it is right now is causing a problem.  Are people passing data that actually needs to be structured cloned (like blobs or something) such that doing a JSON.stringify on it loses information?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Argh, and of course now we need to close up the bug...

Comment 12

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8454292 [details] [diff] [review]
> Patch v1
> 
> When writing the explanation, please
> keep in mind that JSON.stringify() is not idempotent: two consecutive calls
> to it for the same object can return different results.
Even though JSON.stringify() is not idempotent, is there a case that the following scenario could happen?

1. The first call wouldn't indroduce privilege violation in a sandbox with lower privileges. Thus no "Permission Denied" error is thrown so it can get through to send IPC messages.
2. The second consecutive call somehow runs on the same object to do something which should have been banned at the first call.

If not, then the fix looks alright to me.

On the other hand, the root cause of the code as it is right now is that AlarmService.jsm simply uses JSON stringified data without parsing it again when receiving a "AlarmsManager:Add" IPC message [1]. But it only happens to "AlarmsManager:Add" since we only use a sand box to JSON stringify the data in add() in AlarmManager.js. We may consider to JSON parse the "AlarmsManager:Add" message and don't do the same thing for other types of messages. But the logic seems clearer if the changes are kept within AlarmManager.js.

Btw, instead of passing the raw data, would it reduce your concern if we pass JSON.parse(sandbox stringified data) to the IPC message in add() in AlarmManager.js?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#105

Comment 13

[Boris Zbarsky [:bzbarsky]]

> is there a case that the following scenario could happen?

In general, yes.  Say the caller does this:

  var step = 0;
  var obj = { 
    toJSON: function() {
      if (step == 0) {
        ++step;
        return "fluffy kitten";
      } 

      return crossOriginWindow.location;
    } 
  };

Then the first call to JSON.stringify will produce the string "fluffy kitten", while the second will do exactly the same thing that JSON.stringify(crossOriginWindow.location) does.

It's _possible_ that JSObject Xrays save us here, but I'd rather not depend on that.  Especially if we need this backported to branches that don't have object Xrays.

> AlarmService.jsm simply uses JSON stringified data without parsing it again

Aha, thank you.  That makes sense.

How come none of our existing tests in dom/alarm/test caught this issue?  Can we please add tests that would have caught it?

> would it reduce your concern if we pass JSON.parse(sandbox stringified data)

Yes!  At that point we know it's safe.  Basically, we'd be doing what GetParamsForMessage, but doing it with the page's principal, not the system principal.  Good catch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8454292 [details] [diff] [review]
Patch v1

I don't think this is safe.

Comment 18

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v2

Fixing alarms and timer alerts in a safer way and adding a test case covering this issue.

Comment 19

[Andrew McCreight [:mccr8]]

This isn't a security bug per se.  It has been hidden because it is a regression from a security bug, and there's discussion of that other bug here.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8455136 [details] [diff] [review]
Patch v2

r=me.  Sorry for the lag, and thanks for doing this!

Comment 21

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Try run for this patch. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=4fd37a3bcc31

Comment 22

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/7a3c1d05d492

Comment 23

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=43920514&tree=B2g-Inbound&full=1

03:56:15     INFO -  07-16 10:55:04.212 I/Gecko   (  672):
03:56:15  WARNING -  07-16 10:55:04.611 E/GeckoConsole(  672): [JavaScript Error: "NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]" {file: "chrome://specialpowers/content/SpecialPowersObserver.js" line: 96}]
03:56:15     INFO -  07-16 10:55:04.721 I/Gecko   (  672): -*- NetworkService: NetworkService shutdown
03:56:15     INFO -  07-16 10:55:05.092 I/Gonk    (  672): RIL[0]: OnDisconnect
03:56:15     INFO -  07-16 10:55:05.111 I/Gecko   (  672):
03:56:15     INFO -  07-16 10:55:05.111 I/Gecko   (  672): ###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
03:56:15     INFO -  07-16 10:55:05.111 I/Gecko   (  672):
03:56:15     INFO -  07-16 10:55:05.132 I/GonkMemoryPressure(  672): Observed XPCOM shutdown.
03:56:15     INFO -  07-16 10:55:05.601 I/ServiceManager(   33): service 'media.resource_manager' died
03:56:15     INFO -  07-16 10:55:05.601 I/ServiceManager(   33): service 'permission' died
03:56:15     INFO - Return code: 0
03:56:15     INFO - Running command: ['ps', '-C', 'emulator']
03:56:15     INFO - Copy/paste: ps -C emulator
03:56:15     INFO -    PID TTY          TIME CMD
03:56:15    ERROR - Return code: 1

Comment 24

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Hi Carsten,

Based on the link provided in comment 23, I also looked into the testing summary page for this patch.
https://tbpl.mozilla.org/?rev=7a3c1d05d492&tree=B2g-Inbound
That failed item (B2G ICS Emulator Opt mochitest-3) appeared retried couple times and then succeeded. So it looks to me that the failure is more like resulted from the transient stability issue of the test pod. Do you still think back-out is necessary for this patch?

Comment 25

[Carsten Book [:Tomcat]]

Hey Sean,

the "retries" was me because i tried to find out if that test failure was caused by your checkin or someone else. 

The problem with this failure was also that this failed the at least the next 2 pushes after your checkin, so it was a test regression. No idea what we can do more here for the stability of the test pod.

Comment 26

[Sean Lin [:seanlin] (inactive from Mar 2016)]

I've found couple changes which got checked in before this patch also sufferred test failures on the same item (B2G ICS Emulator Opt mochitest-3).

https://tbpl.mozilla.org/?rev=c1b96c4b721b&tree=B2g-Inbound
https://tbpl.mozilla.org/?rev=c16fc5f5a46c&tree=B2g-Inbound
https://tbpl.mozilla.org/?rev=c132a6955856&tree=B2g-Inbound

And they all failed on a random one of the test cases under /tests/content/media/test/ with similar error messages. So it doesn't appear the issue was introduced by this patch. On the other hand, the logic under /content/media doesn't seem quite relevant to this patch. (But it might be still possible that this patch somehow increases the failure rate of this issue even though it already exists.)

Based on the observations stated above, we're inclined to reland this patch. If the issue of  intermittent failure still bothers us to some extent, we may consider to open a separate bug to keep track of it.

Comment 27

[Gene Lian (I already quit Mozilla)]

Comment #26 mentioned this test regression is not caused by this patch. Let's reland.

https://hg.mozilla.org/integration/b2g-inbound/rev/6339cb1c61b2

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6339cb1c61b2

Comment 30

[Josh Schmitt [Joshs]]

Issue does not repro on todays build 7/18/2014, the alarms for Email, Clock and Calendar now work as expected.

Verifying as fixed.

Environmental Variables:
Device: Flame Master
Build ID: 20140718040202
Gaia: Unknown
Gecko: e6e8c93a1b6e
Version: 33.0a1 (Master)
Firmware Version: v122

User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Comment 34

[bhavana bajaj [:bajaj]]

Switching the 2.1?->2.1+, on these fixed bugs as these are regression.

Nothing to land here, its just flag-cleanup of 2.1? list. Please Ni me if there is confusion/disagreement.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

This fix for this was landed on esr31 as part of the uplift of bug 1015540.