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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[sarsenyev]

Attachment: 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

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

> 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?

Comment 2

[Jason Smith [:jsmith]]

(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.

Comment 3

[Jason Smith [:jsmith]]

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.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Close All Notifications button in UI Test doesn't work either. Doesn't cause a crash though. Taking.

Comment 5

[Jason Smith [:jsmith]]

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.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 7

[Jason Smith [:jsmith]]

(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.

Comment 8

[Jason Smith [:jsmith]]

Attachment: 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.

Comment 9

[bhavana bajaj [:bajaj]]

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.

Comment 10

[Jason Smith [:jsmith]]

Attachment: Simple Notification Close Test Case

Comment 11

[Jason Smith [:jsmith]]

Attachment: Test Case

Comment 12

[Natalya Kot [:nkot]]

Attachment: 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?

Comment 13

[Jason Smith [:jsmith]]

(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?

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 15

[Jason Smith [:jsmith]]

(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.

Comment 16

[Jason Smith [:jsmith]]

(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.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

CC'ing reuben on this, since he worked on Bug 901214, which may relate.

Comment 19

[Natalya Kot [:nkot]]

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.

Comment 20

[Reuben Morais [:reuben]]

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.

Comment 21

[Ben Francis [:benfrancis]]

I don't think this lives in Gaia's browser component, thanks for taking this Reuben.

Comment 22

[:gerard-majax]

I do reproduce this bug.

Comment 23

[Reuben Morais [:reuben]]

Attachment: Use CheckPermission in Notifications message handlers

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 25

[Reuben Morais [:reuben]]

(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.

Comment 26

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8ce724c02b7c

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 28

[Reuben Morais [:reuben]]

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?

Comment 29

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/mozilla-central/rev/8ce724c02b7c

Comment 30

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a7c856f4a7f9

Comment 31

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Backed out for mochitest crashes.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c042535f8f8d

https://tbpl.mozilla.org/php/getParsedLog.php?id=30603126&tree=Mozilla-B2g26-v1.2
https://tbpl.mozilla.org/php/getParsedLog.php?id=30603406&tree=Mozilla-B2g26-v1.2

Comment 32

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Looks like it was needs-clobber bustage. Re-landed with a clobber first.

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ce9c8493ad98

Comment 33

[Jason Smith [:jsmith]]

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.