786338 - NotificationHandler sending bad intents to gecko

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Margaret has been seeing some strange behavior with notifications potentially. i.e. a "This url must be opened by an app Alert prompt appears" with nothing to do. I recently changed their behavior slightly in bug 781061. It sounds like we are trying to open the dat url sent to Fennec rather than The intent being sent to Fennec is what I expect:

I/GeckoNotificationHandler(25156): startActivity with intent: Action='org.mozilla.gecko.ACTION_ALERT_CALLBACK appName='org.mozilla.fennec.App'
I/ActivityManager(  308): START {act=org.mozilla.gecko.ACTION_ALERT_CALLBACK dat=alert:/-297100515?name=update-app&app=org.mozilla.fennec.App&cookie= flg=0x10000000 cmp=org.mozilla.fennec/.App u=0} from pid 25156

I'm guessing that we are trying to open this dat term when the Notification is tapped and Gecko isn't running. Will need to confirm that though.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

I hate having to do this this way. If Gecko is running and you tap a Notification, we end up calling onNewIntent which has all this code to do special things with different intents. If not (we used to just bail, but with my recent change we start Gecko now and) we call onCreate which just assumes any url's it finds in an intent are real urls.

I'd really like onCreate to call into the same code that onNewIntent does. Unfortunately, onNewIntent fires intents to start loads whereas onCreate just sends the url to gecko with startup params. i.e. untangling the two is not being easy. Since this is on beta right now, I'd like to put up this patch for there and work on a better one for Nightly?

Comment 2

[:Margaret Leibovic]

Oh, I didn't see you file this. I filed bug 786394. Is that a dupe now?

Comment 3

[Wesley Johnston (:wesj)]

Yeah. I'm still not sure what your webapp bug was though....

Comment 4

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1052d9e756f8

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 656127 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 781061
User impact if declined: Clicking system Notifications after Fennec has closed will lead to strange messages and blank tabs. In webapps it can lead to unusable situations (i.e. a blank tab you can't get out of, but webapps are turned off on beta).
Testing completed (on m-c, etc.): Landed on mc 8-29
Risk to taking this patch (and alternatives if risky): This is the low risk alternative. We could change 781061 so that we don't launch fennec in these situations again, but that requires some Android trickery that is non-trivial on Aurora. We could revert 781061 entirely on Beta since webapps are disabled there.
String or UUID changes made by this patch: None.

Comment 6

[Alex Keybl [:akeybl]]

Let's track the backout of bug 781061 there.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1052d9e756f8

Comment 11

[Aaron Train [:aaronmt]]

Aurora checkin?

Comment 12

[Wesley Johnston (:wesj)]

Sorry for the delay:
https://hg.mozilla.org/releases/mozilla-aurora/rev/d164700915f8

Comment 13

[Cristian Nicolae (:xti)]

Did the patch land on Aurora 09/13?

I installed the Aurora build mentioned above, I checked for updates and I quit app after the notification was triggered. Clearing the notifications will auto-open Aurora.

Comment 14

[Cristian Nicolae (:xti)]

I can reproduce this issue on the Aurora from 09/14. When notification update was triggered, I quit Aurora and then I cleared the update notification from Android Notification Bar. The result was auto-app relaunch. Reopening bug

--
Device: Galaxy Note
OS: Android 4.0.4

Comment 15

[Wesley Johnston (:wesj)]

Ahh. The app relaunching is a different bug than the one fixed here (trying to open a bogus URL). The other piece was fixed in bug 786599, but not pushed to Aurora yet (doing that now). Thanks for catching that. Reclosing.