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

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: kanru, Assigned: selin)

Tracking

unspecified
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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
Created attachment 8583636 [details] [review]
[gaia] seanyhlin:bug-1140275 > mozilla-b2g:master
Created attachment 8583637 [details]
Patch Gaia

Gaia patch - Allow required system messages for mochitest.
Assignee: nobody → selin
Attachment #8583637 - Flags: review?(fabrice)
Created attachment 8583638 [details] [diff] [review]
Patch Gecko, v1

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)
Created attachment 8641439 [details] [diff] [review]
Patch, v2

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)
Created attachment 8642276 [details] [diff] [review]
Patch, v3

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
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.