1508279 - On notification clicking fails to open target URL

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[lipart25]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36

Steps to reproduce:

1) Launch FF on Android
2) Go to https://gauntface.github.io/simple-push-demo
3) Click [Enable push notification] switch
4) Activate "Enable Push Notifications" (and authorize permission prompt)
5) Add payload text "Test" 
6) Click [Send a Push via XHR] button
7) Swipe to display the notifications
8) Press to open the notification


Actual results:

Push notification is closed. Not open target URL.


Expected results:

Push notification is closed. Should open target URL in new tab.

Comment 1

[dmitry.teplyakoff]

Same thing!
Reproduced on Firefox 63.0.2 Android 8.0.0 Samsung Galaxy S9

Comment 2

[lilydjwg]

Same here on Firefox 63.0.2 Android 8.0.0 Sony Xperia XZ2 Compact.

My Firefox on Android has accumulated 26 message notifications from Mastodon since this version that I have no way to clear. Next time Mastodon notifies me about a new message the notification will have 27 items (or lines?): they won't disappear.

Comment 3

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Having push notifications still work sounds pretty important, either P1 or P2. 
Too late to fix in 64 but we could still take a patch for 66/65.

Comment 4

[Bogdan Surd, QA [:bsurd, NI]]

Regression window:
Last good revision: ddd3a8daeb331d69da2dd7547d5bc87fdc97acf1
First bad revision: e16e23725134a5afefc6610fc820011d44be5ac0
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ddd3a8daeb331d69da2dd7547d5bc87fdc97acf1&tochange=e16e23725134a5afefc6610fc820011d44be5ac0

Comment 5

[David Baron :dbaron:]

Yeah, I've been seeing this as well on Facebook's (the mobile web version), where clicking the notifications that come from Firefox doesn't do anything.

Comment 6

[Vlad Baicu]

Seems like a regression caused by bug 1467840, will try and work out a patch for it.

Comment 7

[:sdaswani]

Vlad any ideas why the regression wasn't caught during regression testing, either by Dev or QA team?

Comment 8

[Vlad Baicu]

The patch that caused the regression seems to be from the Oreo series. Usually, we test and also review each other's patches that imply significant changes, however we do not go into much detail.

As for the QA, I can only speculate but as it was a big change they did test against the BFTs set of tests which includes only generic usage of the notification. 

Bogdan, can this case be added to the test suite ?

Comment 9

[Bogdan Surd, QA [:bsurd, NI]]

In order to avoid this in the future, I've added a test case using the link provided in Comment 0 to the BFTs suite so that we will capture problems regarding XHR push notifications.

Comment 12

[Vlad Baicu]

Attachment: Bug 1508279 - Handle persistent notification actions. r=sdaswani

Handle persistent notifications click actions in NotificationReceiver.

Comment 13

[Vlad Baicu]

Upon further investigation some things are not entirely clear to me as notification code goes through gecko in c++. For example, the notifications received with the given STRs are treated as persistent notifications. In Android, these usually are ongoing notifications (e.g. foreground service notifications), however I haven't found docs that could provide more information. I tried to go back to the revision provided in the regression window by Bogdan but I had to revert the entire Oreo series patches as most of those had dependencies on previous patches in order to work properly thus, enlarging the number of patches that might have been responsible for this regression.

Altough my patch doesn't resolve the underlying cause of the issue, in case of persistent notifications we still initialize gecko but handle their actions in the NotificationReceiver, this being a speculative fix.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c8923903d73
Handle persistent notification actions. r=geckoview-reviewers,snorp

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9c8923903d73

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. We should also get QA to verify that things are working better with this patch.

Comment 17

[Vlad Baicu]

Because this is a speculative fix, I am not entirely confident of uplifting this patch to beta without QA verification, going to wait on their input first.

Comment 18

[Andrei Bodea[:andrei]]

Hello,
I re-tested this issue on the following builds: FF 66.0a1(latest Nightly) and FF 62.0.3 with a Samsung Galaxy Note 9(Android 8.1.0) and the issue can still be reproduced on the latest nightly.

Upon further investigation it can be noticed the following:

FF 66.0a1(latest Nightly) Opening the push notification while the FF is in background will resume the FF and no new page/tab is opened.
Opening the push notification when the FF is in foreground nothing happens, no new page/tab is opened (see the video).
FF 62.0.3 When opening the push notification while the FF is in background or foreground it will open a new page/tab everytime (see the video).

Note that for FF 66.0a1 is not working at all, the app is only resumed if tapping on a push notification.

Thanks,
Andrei

Comment 19

[Vlad Baicu]

Thank you for the input Andrei, reopening the issue and will continue working on a new patch.

Comment 20

[Vlad Baicu]

Attachment: Bug 1508279 - Use the correct intent for GeckoServicesCreatorService. r=sdaswani

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e29be447ca75
Use the correct intent for GeckoServicesCreatorService. r=sdaswani

Comment 22

[Andreea Pavel [:apavel]]

(In reply to Pulsebot from comment #14)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c8923903d73
Handle persistent notification actions. r=geckoview-reviewers,snorp

Backed out on request.

Backout: https://hg.mozilla.org/integration/autoland/rev/64aa8ab3332b0f84707424779368b5ace23ec761

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e29be447ca75

Comment 24

[Ryan VanderMeulen [:RyanVM]]

NI Andrei to hopefully verify this fix in time to get it into the Fennec 65 RC build later this week.

Comment 25

[Andrei Bodea[:andrei]]

Verified as fixed on the latest Nightly build 66.0a1 following the scenarios from Comment 18 with Samsung Galaxy Note 9 (Android 8.1.0).
A new page was opened every time when I tapped on the push notification even when the Fennec was closed (video), everything looks good!

Comment 26

[Vlad Baicu]

Comment on attachment 9037604 [details]
Bug 1508279 - Use the correct intent for GeckoServicesCreatorService. r=sdaswani

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1467840

User impact if declined: Notification actions will not work.

Is this code covered by automated tests?: Unknown

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: Steps to reproduce:

1. Launch FF on Android
2. Go to https://gauntface.github.io/simple-push-demo
3. Click [Enable push notification] switch
4. Activate "Enable Push Notifications" (and authorize permission prompt)
5. Add payload text "Test"
6. Click [Send a Push via XHR] button
7. Swipe to display the notifications
8. Press to open the notification

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The regression has been caused by a wrong parameter. The patch only fixes that issue.

String changes made/needed:

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9037604 [details]
Bug 1508279 - Use the correct intent for GeckoServicesCreatorService. r=sdaswani

[Triage Comment]
Fixes issues with notification actions. Approved for Fennec 65.0 RC1.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/305c7368c03b

Comment 29

[miralobontiu]

Verified as fixed on RC 65.0.
Thank you!