Alarm API - Follow-Up Fix for System Message Integration

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: Gene Lian (I already quit Mozilla))

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Need to add sanity checks to avoid adding alarms when the app's manifestURL is null. Please see Fabrice's suggestion at https://bugzilla.mozilla.org/show_bug.cgi?id=749551#c130.
(Assignee)

Updated

5 years ago
Assignee: nobody → clian
Blocks: 749551
(Assignee)

Updated

5 years ago
Summary: Alarm API - Avoid Adding Alarms when App's ManifestURL is Null → Alarm API - Follow-Up Fix for System Message Integration
(Assignee)

Comment 1

5 years ago
Created attachment 642849 [details] [diff] [review]
WIP Patch

Fabrice,

May I invite you to be the reviewer of the System Message integration for Alarm APIs? This following summarizes what I've changed in this patch:

1. In the content process (AlarmsManager.js), the aWindow.document.nodePrincipal.URI.spec is saved and sent to parent process when adding alarms.

2. In the parent process (AlarmService.jsm), when alarm is fired, I used Services.io.newURI() to create a new manifestURI for sending system messages.

3. Add a sanity check to avoid adding alarms from a page without valid URI.

4. s/manifestURL/URISpec


Questions:

1. As discussed in the e-mail, it seems the the aManifestURI.spec passed into SystemMessageInternal.sendMessage() is still http://clock.gaiamobile.org/, instead of http://clock.gaiamobile.org/manifest.webapp as expected. Is it correct to use aWindow.document.nodePrincipal.URI.spec to get URI?

2. Although the aManifestURI.spec could be wrong, it seems the SystemMessageManager has never been started because the debug messages within the SystemMessageManager.init() didn't show (debug is already turned on). That's why SystemMessageManager.receiveMessage() cannot receive any messages sent from ppmm.sendAsyncMessage("SystemMessageManager:Message", ...). How should we correctly launch the SystemMessageManager?

Thanks,
Gene
Attachment #642849 - Flags: review?(fabrice)
(Assignee)

Updated

5 years ago
Attachment #642849 - Flags: review?(fabrice)
(Assignee)

Comment 2

5 years ago
Created attachment 642892 [details] [diff] [review]
Patch

Fabrice,

May I invite you to be the reviewer of the System Message integration for Alarm APIs? The following summarizes what I've changed in this patch:

1. In the content process (AlarmsManager.js), the |utils.getApp().manifestURL| is saved and sent to parent process when adding alarms.

2. In the parent process (AlarmService.jsm), when alarm is fired, I used Services.io.newURI() to create a new manifestURI for sending system messages. Don't need to check if the manifestURL is null or not because of item #3.

3. Disable the Alarm APIs (i.e. set navigator.mozAlarms to null) when the page has no valid manifestURL, so that clients won't have chances to add an alarm that had no manifestURL.

Thanks,
Gene
Attachment #642849 - Attachment is obsolete: true
Attachment #642892 - Flags: review?(fabrice)
(Assignee)

Comment 3

5 years ago
Comment on attachment 642892 [details] [diff] [review]
Patch

Oops... This one cannot be passed by Mochitest which doesn't always use installed apps for testing. I'll upload another solution later. Sorry for bugging.

Gene
Attachment #642892 - Flags: review?(fabrice)
(Assignee)

Comment 4

5 years ago
Created attachment 642900 [details] [diff] [review]
Patch, V2

Fabrice,

May I invite you to be the reviewer of the System Message integration for Alarm APIs? The following summarizes what I've changed in this patch:

1. In the content process (AlarmsManager.js), the |utils.getApp().manifestURL| is saved and sent to parent process when adding alarms.

2. In the parent process (AlarmService.jsm), when alarm is fired, I used Services.io.newURI() to create a new manifestURI for sending system messages. Don't need to check if the manifestURL is null or not because of item #3.

3. Add an sanity check to avoid adding alarms for an non-installed app which can ensure all the added alarms are associated with an valid manifestURL for sending system messages.

Thanks,
Gene
Attachment #642892 - Attachment is obsolete: true
Attachment #642900 - Flags: review?(fabrice)
Attachment #642900 - Flags: review?(fabrice) → review+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: Try-server passed: https://tbpl.mozilla.org/?tree=Try&rev=978817c3c6ee
http://hg.mozilla.org/integration/mozilla-inbound/rev/44e8a79e4be3
Keywords: checkin-needed
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/44e8a79e4be3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: Try-server passed: https://tbpl.mozilla.org/?tree=Try&rev=978817c3c6ee → Try-server passed: https://tbpl.mozilla.org/?tree=Try&rev=978817c3c6ee
(Assignee)

Updated

5 years ago
Whiteboard: Try-server passed: https://tbpl.mozilla.org/?tree=Try&rev=978817c3c6ee
You need to log in before you can comment on or make changes to this bug.