899141 - URLs are not opened from other apps if the "Don't keep activities" option is set

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Nightly 25.0a1 2013-07-28
Asus EEE Transformer TF101 (Android 4.0.4)

Steps to reproduce:
1) Set the "Don't keep activities" developer option to ON
2) Open Firefox and load any website
3) Minimize the app
4) Open a link from another app (Twitter, Gmail app) using Firefox

Expected result:
The website is loaded in a new tab

Actual result:
No new tabs are created and the website is never loaded

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Always open a startup tab if given a URL

If we're already running, we skip over the call to loadStartupTab(), but we shouldn't do that if we're given a new URL to load.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 784425 [details] [diff] [review]
Always open a startup tab if given a URL

Add a comment above the "if" block:
// External URLs should always be loaded regardless of how we are restoring

(or something like that)

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 784425 [details] [diff] [review]
Always open a startup tab if given a URL

Review of attachment 784425 [details] [diff] [review]:
-----------------------------------------------------------------

Thinking about this more, this will still lead to problems. Consider the following STR:

1) Launch Fennec with an intent to open some URL
2) Cause Fennec to be killed in the background
3) Reopen Fennec from the recent apps list

Fennec will be reopened in step 3 without a new intent, meaning it will still use the intent from step 1. And since the activity gets recreated, this code path gets hit again. Note that the proper fix here, whatever it will be, may overlap with bug 896170.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Fix startup intent handling

I think we can simplify our intent handled logic by simply checking for the existence of savedInstanceState. savedInstanceState will be non-null after the activity is killed (either with DKA or Fennec being background killed), and in those cases, onCreate() will always be called with the prior intent (and the new intent is given to onNewIntent(), which is called immediately after).

I removed some old code from bug 732268 that checked if we were being launched from the recent apps list. We don't need this anymore since we're clearing the old intent in onCreate(). I also removed our ACTION_MAIN handling since that only creates an empty URI load event, which doesn't actually do anything.

Mark suggested adding some tests for these, which I think is a good idea. Though we probably won't be able to enable Don't Keep Activities directly in robocop, we might be able to add special code in onDestroy() to prevent killing Gecko when the activity gets destroyed. That would allow us to create the activity again, essentially reproducing DKA's behavior.

Also including Kats for review since he reviewed bug 896992.

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Mark suggested adding some tests for these, which I think is a good idea.

Filed bug 900799.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 784739 [details] [diff] [review]
Fix startup intent handling

The approach seems like it should work and is cleaner.

I assume you have tested the cases where you are removing code to make sure the bugs fixed by that code are still fixed.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 784739 [details] [diff] [review]
Fix startup intent handling

Review of attachment 784739 [details] [diff] [review]:
-----------------------------------------------------------------

This is definitely much nicer, if it works! I'm a little concerned that this might break existing codepaths that we rely on to restore tabs when going back into Fennec, but I guess if session restore handles all of those cases then it should be fine. It should at least be simpler to update the session restore code to restore things if this does break stuff since the logic is simpler now. r=me but this will need a good amount of testing. I'll be sure to run with DKA once this lands to test as well.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Also CC'ing liuche as FYI, since this basically removes all the code she added recently.

Comment 9

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/de0b096994cf

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 12

[u421692]

Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Nightly 26.0a1 (2013-08-15)

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 784739 [details] [diff] [review]
Fix startup intent handling

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: tabs may not load if don't keep activities is enabled
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk; baked on m-c for several weeks
String or IDL/UUID changes made by this patch: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/478206fb256f

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0b1 (2013-09-18)
Device: LG Nexus 4 
OS: Android 4.2.2