[B2G][Notification] Browser app does not handle Notification events/messages correctly

VERIFIED FIXED in Firefox 28, Firefox OS v1.2

Status

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: sarsenyev, Assigned: reuben)

Tracking

unspecified
1.3 Sprint 5 - 11/22
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: burirun1 [systemsfe], burirun3)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 809541 [details]
logcat.txt

Description:
Cannot close "Notification" bar, when clicking on the "Close" button in the  Notifications API

Repro Steps:
1) Updated Buri/Inari/Leo/Unagi to Build ID: 20130924004002
2) Go to http://mozilla.github.io/qa-testcase-data/webapi/notifications/index.html
3) After the permission is granted tap the "Request permisson" bar
3) Tap the "Show Notification" bar
4) After the new notification appears tap the "Close" button using the notification index

Actual:
The "Close" button doesn't respond

Expected:
The notification disappears from the status bar with on close fired in logcat

Environmental Variables
Build ID: 20130924004002
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/b34384409be6
Gaia: a13c76f8d3c617ee57c302c103da04ed1a6298d1
Platform Version: 26.0a2

Notes:
Repro frequency: 100%
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-id=9942
See the logcat
Whiteboard: burirun1 → burirun1 [systemsfe]
blocking-b2g: --- → koi?
Component: Gaia::Settings → Gaia::System

Updated

5 years ago
Blocks: 911002
> var notificationID = parseInt(document.getElementById('notificationId').value);

IDs are one of our multiple names for tags. Tags are strings, either a UUID (for desktop notification) or a combination of origin and user defined tag (for new notifications). Why are you parsing an int from that?
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #1)
> > var notificationID = parseInt(document.getElementById('notificationId').value);
> 
> IDs are one of our multiple names for tags. Tags are strings, either a UUID
> (for desktop notification) or a combination of origin and user defined tag
> (for new notifications). Why are you parsing an int from that?

It's just part of the test app to grab one of the notifications created and operate on it with a close function call. Each notification I create I store in a variable called notificationList. Then, when close is called, I grab the index provided from user input and close the notification object from the list.
I can reproduce on a 1.2 build. When I hit close with the correct index provided, I get a page to appear to say "well this is embarrassing" and in one case, a crash.
Close All Notifications button in UI Test doesn't work either. Doesn't cause a crash though. Taking.
Crash report I got in one case was this - https://crash-stats.mozilla.com/report/index/118f9287-c57c-4ab0-8cb5-035802130925. Although the stack is lacking symbols entirely, so it's probably not going to be that useful. Haven't been able to get the crash since - every other time I've tested this I got the well this is embarrassing page.

Updated

5 years ago
blocking-b2g: koi? → koi+

Updated

5 years ago
Keywords: regression, regressionwindow-wanted

Updated

5 years ago
QA Contact: nkot
Ok, just checked on desktop. Both original test and uitest app works for me with clean m-c master, clean gaia master (notification closes). My earlier test in comment 4 was using a dirty m-c master with some changes, which may've regressed something.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6)
> Ok, just checked on desktop. Both original test and uitest app works for me
> with clean m-c master, clean gaia master (notification closes). My earlier
> test in comment 4 was using a dirty m-c master with some changes, which
> may've regressed something.

Did you try testing on device? 

FWIW - The regression I saw when I tested this looked like a Gecko regression, not a Gaia regression.
Created attachment 810270 [details]
Logcat 2

Here's a 2nd logcat with more information. Logcat is showing:

09-25 17:46:49.333: I/Gecko(111): Security problem: Content process does not have `desktop-notification'.  It will be killed.

Which implies a permissions issue at the DOM level.

Updated

5 years ago
Component: Gaia::System → DOM
Product: Boot2Gecko → Core
Version: unspecified → Trunk

Updated

5 years ago
QA Contact: nkot
NI on Kyle to help with next steps here.

Kyle given this is assigned to you and considering jason's comment 8, unclear if this needs a new assignee or you are working on this. Can you please help with the info or directing this to the right folks.
Flags: needinfo?(kyle)
Created attachment 816844 [details]
Simple Notification Close Test Case

Updated

5 years ago
Attachment #816844 - Attachment is obsolete: true
Created attachment 816845 [details]
Test Case

Comment 12

5 years ago
Created attachment 817264 [details]
Logcat 3

I was trying to find a regression window here... So far have tested builds from 09/24 (the day when bug was filed) to 09/16 - the issue reproduces on all of them

- cannot close the notification using the "Close"  button

STR:
1.Go to http://mozilla.github.io/qa-testcase-data/webapi/notifications/index.html
2.Tap request permission
3.Once granted, tap "Show Notification" button
4.Tap "Close" button

Tried also entering "0" into a Index field, still not able to close notifications, got crash a few times.

However, I've never got "well this is embarrassing page" in my results (see comment 3 and comment 5) - when i hit "Close" either nothing happens or crash. 

Jason,
Should I continue testing further down? any corrections?
(In reply to nkot from comment #12)
> Created attachment 817264 [details]
> Logcat 3
> 
> I was trying to find a regression window here... So far have tested builds
> from 09/24 (the day when bug was filed) to 09/16 - the issue reproduces on
> all of them
> 
> - cannot close the notification using the "Close"  button
> 
> STR:
> 1.Go to
> http://mozilla.github.io/qa-testcase-data/webapi/notifications/index.html
> 2.Tap request permission
> 3.Once granted, tap "Show Notification" button
> 4.Tap "Close" button
> 
> Tried also entering "0" into a Index field, still not able to close
> notifications, got crash a few times.
> 
> However, I've never got "well this is embarrassing page" in my results (see
> comment 3 and comment 5) - when i hit "Close" either nothing happens or
> crash. 
> 
> Jason,
> Should I continue testing further down? any corrections?

A crash in what sense? What are you seeing when this happens? A crash report dialog?
v1.2 Nexus 4 build. UI Tests "Close All Notification" button works, closing notifications in utility tray as expected, and firing all onclose events for the notifications. All close integration tests pass on desktop. Might this be a bug in the test program? Closing as WORKSFORME.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(kyle)
Resolution: --- → WORKSFORME
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #14)
> v1.2 Nexus 4 build. UI Tests "Close All Notification" button works, closing
> notifications in utility tray as expected, and firing all onclose events for
> the notifications. All close integration tests pass on desktop. Might this
> be a bug in the test program? Closing as WORKSFORME.

No. The reduced test case already shows this not working, so you didn't test this correctly.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(In reply to Jason Smith [:jsmith] from comment #15)
> (In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #14)
> > v1.2 Nexus 4 build. UI Tests "Close All Notification" button works, closing
> > notifications in utility tray as expected, and firing all onclose events for
> > the notifications. All close integration tests pass on desktop. Might this
> > be a bug in the test program? Closing as WORKSFORME.
> 
> No. The reduced test case already shows this not working, so you didn't test
> this correctly.

UI Tests will test web notifications specifically in a packaged app context, not a browser context. So you aren't executing the right STR here.
Summary: [B2G][Notification] Notifications API -The "Close" button doesn't respond → [B2G][Notification] Browser app does not handle Notification events/messages correctly
Component: DOM → Gaia::Browser
Product: Core → Boot2Gecko
Version: Trunk → unspecified
CC'ing reuben on this, since he worked on Bug 901214, which may relate.

Comment 19

5 years ago
I was able to reproduce this issue using the Test Case from comment 11, 
- getting the same result as described above ("well this is embarrassing page")
- seeing the same error in my logs 

10-15 17:24:21.969: I/Gecko(140): Security problem: Content process does not have `desktop-notification'.  It will be killed.

The issue reproduces on all tested builds up to 08/26 (build ID: 20130826163255) when the feature initially landed. 
So this is not a regression then.
Keywords: regression, regressionwindow-wanted
(Assignee)

Comment 20

5 years ago
Yea, this is the same problem that appeared in bug 901214. ContentParent doesn't know how to handle permissions for web pages. I have a patch to fix this.
Assignee: kyle → reuben.bmo
I don't think this lives in Gaia's browser component, thanks for taking this Reuben.
Component: Gaia::Browser → General
(Assignee)

Updated

5 years ago
Depends on: 904298
(Reporter)

Updated

5 years ago
Whiteboard: burirun1 [systemsfe] → burirun1 [systemsfe], burirun3
I do reproduce this bug.
(Assignee)

Comment 23

5 years ago
Created attachment 827802 [details] [diff] [review]
Use CheckPermission in Notifications message handlers
Attachment #827802 - Flags: review?(jonas)
Comment on attachment 827802 [details] [diff] [review]
Use CheckPermission in Notifications message handlers

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

r=me with the below answered.

::: dom/ipc/ContentParent.cpp
@@ +2907,5 @@
> +#ifdef MOZ_CHILD_PERMISSIONS
> +    uint32_t permission = mozilla::CheckPermission(this, aPrincipal,
> +                                                   "desktop-notification");
> +    if (permission != nsIPermissionManager::ALLOW_ACTION) {
> +        return true;

You are changing this from "return false" to "return true". Was that intentional? What effect will that have.

@@ +2915,4 @@
>      nsCOMPtr<nsIAlertsService> sysAlerts(do_GetService(NS_ALERTSERVICE_CONTRACTID));
>      if (sysAlerts) {
>          sysAlerts->ShowAlertNotification(aImageUrl, aTitle, aText, aTextClickable,
> +                                         aCookie, this, aName, aBidi, aLang, nullptr);

I think it would make sense to forward the principal here too, even though it won't be used. You have the information available easily and it might be useful in the future.

@@ +2935,3 @@
>      nsCOMPtr<nsIAlertsService> sysAlerts(do_GetService(NS_ALERTSERVICE_CONTRACTID));
>      if (sysAlerts) {
> +        sysAlerts->CloseAlert(aName, nullptr);

Same here.
Attachment #827802 - Flags: review?(jonas) → review+
(Assignee)

Comment 25

5 years ago
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #24)
> ::: dom/ipc/ContentParent.cpp
> @@ +2907,5 @@
> > +#ifdef MOZ_CHILD_PERMISSIONS
> > +    uint32_t permission = mozilla::CheckPermission(this, aPrincipal,
> > +                                                   "desktop-notification");
> > +    if (permission != nsIPermissionManager::ALLOW_ACTION) {
> > +        return true;
> 
> You are changing this from "return false" to "return true". Was that
> intentional? What effect will that have.

Yes, it'll noop instead of killing the child if it doesn't have the permission.
But how can we get into that function if we haven't already in the child process checked that that origin has permission to use notifications?
(Assignee)

Comment 28

5 years ago
My idea is to eventually make it so that ContentParent::ShowAlertNotification would handle prompting if necessary, so you wouldn't have to check the permission before calling nsAlertsService::ShowAlertNotification. Is that not a good idea, or not enough reason to change this behavior now?
Target Milestone: --- → 1.3 Sprint 5 - 11/22
https://hg.mozilla.org/mozilla-central/rev/8ce724c02b7c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: verifyme

Updated

5 years ago
QA Contact: jsmith
status-b2g-v1.2: --- → fixed
status-firefox26: --- → wontfix
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
Flags: needinfo?(reuben.bmo)
Looks like it was needs-clobber bustage. Re-landed with a clobber first.

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ce9c8493ad98
status-b2g-v1.2: affected → fixed
Flags: needinfo?(reuben.bmo)
Keywords: branch-patch-needed
Verified on trunk through an exploratory test pass. Mostly okay, although there was one bug I found that might be a followup to this bug - see bug 943698.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.