Closed Bug 1140275 Opened 5 years ago Closed 4 years ago

System Message API: message is sent to an app page even if not registered in manifest

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: kanru, Assigned: selin)

Details

Attachments

(1 file, 4 obsolete files)

Steps to reproduce:

  1. Edit gaia/apps/clock/manifest.webapp and remove the "messages" section
  2. Re-install gaia (make reset-gaia)
  3. Open clock app and schedule a short alarm

Expected result:
  Alarm does not alarm

Actual result:
  Alarm alarms at the specified time

However if we kill the clock app after step 3, the alarm does not alarm.

See also bug 1137722 comment 10
Attached file Patch Gaia (obsolete) —
Gaia patch - Allow required system messages for mochitest.
Assignee: nobody → selin
Attachment #8583637 - Flags: review?(fabrice)
Attached patch Patch Gecko, v1 (obsolete) — Splinter Review
Patch Gecko v1: Only send messages to pages registered in manifests.
Attachment #8583638 - Flags: review?(fabrice)
Sean, can you explain the changes?
According to bug 1137722 comment 10, when we strictly prevent system messages from sending to app pages not registered in manifests, it would cause test failures on B2G emulators because the page URLs of those tests are not put in the manifest of the mochitest container app. So the Gaia patch is to whitelist those test pages.

However, the URLs of those test pages may end with query strings like 

?autorun=1&timeout=300&closeWhenDone=1&logFile=%2Fdata%2Flocal%2Ftests%2Flog%2Fmochitest.log&fileLevel=INFO&consoleLevel=INFO&hideResultsTable=1&dumpOutputDirectory=%2Ftmp&manifestFile=tests.json

Thus the patch also trims the tailing query string in the page URL registered for system messages. And it looks compatible with the actual usage, such as /dialer/index.html#keyboard-view, in Gaia apps.
I don't agree with doing that. It's perfectly valid to have different query strings for different messages with the same path. We need a better fix.

Also make sure that this addresses the issue from comment #0 which looks slightly different to me.
Attachment #8583637 - Flags: review?(fabrice) → review+
Attachment #8583638 - Flags: review?(fabrice)
Attached patch Patch, v2 (obsolete) — Splinter Review
Prevent system messages from being sent to app pages not registered in manifests. And fix relevant tests. (In comment 0, the generated system message to fire alarms may still be sent to the alarm page even when the page URL hasn't been listed in the app manifest.)
Attachment #8583636 - Attachment is obsolete: true
Attachment #8583637 - Attachment is obsolete: true
Attachment #8583638 - Attachment is obsolete: true
Attachment #8641439 - Flags: review?(fabrice)
Attached patch Patch, v3Splinter Review
Fixing two more requestsync tests.
Attachment #8641439 - Attachment is obsolete: true
Attachment #8641439 - Flags: review?(fabrice)
Attachment #8642276 - Flags: review?(fabrice)
Comment on attachment 8642276 [details] [diff] [review]
Patch, v3

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

Thanks!
Attachment #8642276 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/f85097eaf255
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.