607939 - fail to opened a URL / link from another application after launching

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

STR:

1. Make sure Fennec isn't running (kill the task, whatever)
2. Go to another application like email or twitter that shows links
3. Tap on a link and choose Fennec as the application to handle it

Expected: Fennec loads and opens that link in a tab
Actual: Fennec loads and shows about:home

(the bug summary sucks, please re summarize)

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #0)
> (the bug summary sucks, please re summarize)

Its hard to summarize.

I'd like to make clear that if fennec is already running, the link from the external application is opened correctly. This only happens if fennec isn't already running.

Comment 2

[Dave Townsend [:mossop]]

I see this too, though it isn't 100% reproducable for me.

Comment 3

[[:fabrice] Fabrice Desré]

This case should be managed by the Command Line Handler (along with some code in browser.js). Maybe it's conflicting with android Intent system?

Comment 4

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

Not sure if this helps, but at the moment I have Android asking me which browser I want to use.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

I have seen this, but when I tried to dig into it earlier this week I couldn't reproduce. Obviously Shaver is seeing it this morning, so we can't resolve it yet.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

anything i should be getting from the device that will be useful? 

cant seem to paste abput:support contents in here sorry.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

could you grab the android log please? (adb logcat from the command line to get it over usb)

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

if i have it set to ask me every time, i see this problem. if i male fennec tge default it works. does that help? sorry for typing the textarea doesmt update in real time and doesmt suggest/correct.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: logging patch

I'd like to add some logging to help figure this out.

Comment 14

[Matt Brubeck (:mbrubeck)]

Steps to reproduce from the duplicate bug:
1. Launch Fennec by tapping its launcher icon.
2. Open other apps until memory is exhausted and the OS kills Fennec.
3. Open an link from another app.

Comment 15

[Michael Wu [:mwu]]

Comment on attachment 488721 [details] [diff] [review]
logging patch

r+ for the geckoevent dropping logging

Comment 16

[Michael Wu [:mwu]]

Attachment: Start gecko in onStart

The intent is not properly set when we're in onCreate. This patch moves the initialization code to onStart, which is when the intent is set correctly.

Comment 17

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/815db67d28f1

Comment 18

[Michael Wu [:mwu]]

Hm this may not have fixed all the problems. Reopening - will take another look tomorrow.

Comment 19

[Michael Wu [:mwu]]

Attachment: Try very hard to catch the right intent

This one is pretty extensively tested.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 492023 [details] [diff] [review]
Try very hard to catch the right intent

when can mPreLaunch be true? If not, get rid of it.
what's the relationship between mLaunched and sGeckoRunning? might it better to combine those into one enum? Also, we could get rid of mLaunchButton with an enum.

>         if (mLaunchButton != null || launch(intent))
>            return;

this seems a bit odd to me. If launch fails we'll start trying to send events to gecko. Seems better to check out state (mLaunched or a potential enum) rather than basing this on the return value of launch().

not entirely related, but should we be calling super.onNewIntent()?

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

updated to use enum

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 492847 [details] [diff] [review]
patch

Does a launch when we are GeckoRunning or Launched really mean we should treat this as a resume?


Might be a good time to factor out the reading from /proc/cpuinfo into its own static method.

Its not really clear why we have a separate state for GeckoRunning and for Launched.  I guess GeckoExiting is also another state that is just really PreLaunch.  And I don't see where Launching is used.

Would it make sense to simplify to:

PreLaunch -> (WaitButton) -> Launched

When onXreExit is called, we set the mLaunchState
 to GeckoExiting.  If this happens, and we immediately get a new  intent request, and call back into GeckoAppShell.runGecko.  I am wondering if we need to prevent this.

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #22)
> Comment on attachment 492847 [details] [diff] [review]
> patch
> 
> Does a launch when we are GeckoRunning or Launched really mean we should treat
> this as a resume?
the return value of launch means whether or not the intent has been handled. If the launch code doesn't get all the way to the point that it runs gecko with some args then it should return false and we'll fire off a GeckoEvent to do the load


> Its not really clear why we have a separate state for GeckoRunning and for
> Launched.  
GeckoRunning means that the libraries have been loaded and you can call native methods. Launched is the state just before that time but after launch() has been called

> I guess GeckoExiting is also another state that is just really
> PreLaunch.  And I don't see where Launching is used.

if we have exited we shouldn't try to restart with the same process, hence GeckoExiting != PreLaunch. GeckoExisting essentially means we're going away, don't do anything

which we don't check for Launching explicitly, it is used in that its !PreLaunch and !Launched
 
> 
> Would it make sense to simplify to:
> 
> PreLaunch -> (WaitButton) -> Launched
no
> When onXreExit is called, we set the mLaunchState
>  to GeckoExiting.  If this happens, and we immediately get a new  intent
> request, and call back into GeckoAppShell.runGecko.  I am wondering if we need
> to prevent this.
looks like we should check for this

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 25

[Doug Turner (:dougt)]

> PreLaunch -> (WaitButton) -> Launched

Sorry if I am missing something, but I want to understand these state flags.  Maybe I should have asked "Why doesn't this make sense?".

1) What does the state "Launching" give you?  Unless I am missing something, this state is only set and never check for.

2) What does the state "Gecko*" give you?  Do calls on GeckoAppShell not work until you hit GeckoRunning?

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #25)
> > PreLaunch -> (WaitButton) -> Launched
> 
> Sorry if I am missing something, but I want to understand these state flags. 
> Maybe I should have asked "Why doesn't this make sense?".
> 
> 1) What does the state "Launching" give you?  Unless I am missing something,
> this state is only set and never check for.

launching is not PreLaunch, so it doesn't pass the check on line 160 and it is not Launched, so it passes the check on lines 96-98. The point here is that we only run the code after those checks once, and Launching lets us do that. 
> 
> 2) What does the state "Gecko*" give you?  Do calls on GeckoAppShell not work
> until you hit GeckoRunning?

calls to the native methods on GeckoAppShell are not safe until GeckoRunning is set. GeckoExiting means Gecko is no longer running and isn't expected to run in this process again, so don't wait for it

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 492905 [details] [diff] [review]
patch

talked to dougt on irc, he wants the checking an setting of mLaunchState to be protected from race conditions

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 31

[Doug Turner (:dougt)]

Comment on attachment 493117 [details] [diff] [review]
patch


>     public static boolean mFullscreen = false;
>+    
>     ProgressDialog mProgressDialog;

remove extra ws on the new line.  you have a bunch in this patch.



>+
>+    synchronized boolean checkAndSetLaunchState(LaunchState checkState, LaunchState setState) {
>+        if (mLaunchState != checkState)
>+            return false;
>+        mLaunchState = setState;
>+        return true;
>+    }

Want to add a small comment as to what this returns if checkState doesn't match.



>+            return;
>+
>+
>+        checkAndLaunchUpdate();

1 return only.



Do we have any tests for this? Litmus too?

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/23e28db3b63f

Comment 33

[Doug Turner (:dougt)]

This cased regression bug 614801.  I backed this out:

http://hg.mozilla.org/mozilla-central/rev/eaf1cd30f172

Comment 34

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: follow up patch

the state needs to be static to persist across activities in the same process

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: combined patch

patch for checkin

Comment 36

[Doug Turner (:dougt)]

pushed to mc: http://hg.mozilla.org/mozilla-central/rev/2c41d108edb8

Comment 37

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101129 Namoroka/4.0b8pre Fennec/4.0b3pre

Comment 38

[Ayan Shah[:ashah]]

https://litmus.mozilla.org/show_test.cgi?id=15033