Closed Bug 1508279 Opened 3 years ago Closed 3 years ago

On notification clicking fails to open target URL

Categories

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

ARM
Android
defect

Tracking

(firefox63 wontfix, firefox64 wontfix, firefox65+ verified, firefox66+ verified)

VERIFIED FIXED
Firefox 66
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 + verified
firefox66 + verified

People

(Reporter: lipart25, Assigned: vlad.baicu)

References

Details

Attachments

(2 files)

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.
Same thing!
Reproduced on Firefox 63.0.2 Android 8.0.0 Samsung Galaxy S9
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → ARM
Version: Firefox 63 → Trunk
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.
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.
Regression window:
Last good revision: ddd3a8daeb331d69da2dd7547d5bc87fdc97acf1
First bad revision: e16e23725134a5afefc6610fc820011d44be5ac0
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ddd3a8daeb331d69da2dd7547d5bc87fdc97acf1&tochange=e16e23725134a5afefc6610fc820011d44be5ac0
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.
Blocks: 1467848
Assignee: nobody → vlad.baicu
Seems like a regression caused by bug 1467840, will try and work out a patch for it.
Vlad any ideas why the regression wasn't caught during regression testing, either by Dev or QA team?
Flags: needinfo?(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 ?
Flags: needinfo?(vlad.baicu) → needinfo?(bogdan.surd)
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.
Flags: needinfo?(bogdan.surd)
Duplicate of this bug: 1513728
Duplicate of this bug: 1517454
Handle persistent notifications click actions in NotificationReceiver.

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.

Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c8923903d73
Handle persistent notification actions. r=geckoview-reviewers,snorp

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

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.

Flags: qe-verify+
Flags: needinfo?(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.

Flags: needinfo?(vlad.baicu) → needinfo?(andrei.bodea)

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

Flags: needinfo?(andrei.bodea)

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 66 → ---
Keywords: checkin-needed

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e29be447ca75
Use the correct intent for GeckoServicesCreatorService. r=sdaswani

Keywords: checkin-needed

(In reply to Pulsebot from comment #14)

Pushed by csabou@mozilla.com:
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

Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

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

Flags: needinfo?(andrei.bodea)

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!

Flags: needinfo?(andrei.bodea)

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:

Attachment #9037604 - Flags: approval-mozilla-release?
Attachment #9037604 - Flags: approval-mozilla-beta?

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.

Attachment #9037604 - Flags: approval-mozilla-release?
Attachment #9037604 - Flags: approval-mozilla-release+
Attachment #9037604 - Flags: approval-mozilla-beta?

Verified as fixed on RC 65.0.
Thank you!

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.