Closed Bug 920302 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

VERIFIED FIXED
1.3 Sprint 5 - 11/22
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: sarsenyev, Assigned: reuben)

References

Details

(Whiteboard: burirun1 [systemsfe], burirun3)

Attachments

(5 files, 1 obsolete file)

Attached file 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
> 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.
Assignee: nobody → kyle
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.
blocking-b2g: koi? → koi+
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.
Attached file 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.
Component: Gaia::System → DOM
Product: Boot2Gecko → Core
Version: unspecified → Trunk
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)
Attachment #816844 - Attachment is obsolete: true
Attached file Test Case
Attached file 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
Closed: 9 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.
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.
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
Depends on: 904298
Whiteboard: burirun1 [systemsfe] → burirun1 [systemsfe], burirun3
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+
(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?
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
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
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
Flags: needinfo?(reuben.bmo)
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.