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

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: beltzner, Assigned: blassey)

Tracking

Trunk
All
Android
Bug Flags:
in-testsuite ?
in-litmus +

Details

Attachments

(1 attachment, 9 obsolete attachments)

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)
(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.
tracking-fennec: --- → 2.0b3+
I see this too, though it isn't 100% reproducable for me.
Flags: in-testsuite?
Flags: in-litmus?
Hardware: x86 → All
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?
Not sure if this helps, but at the moment I have Android asking me which browser I want to use.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 609612
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.
anything i should be getting from the device that will be useful? 

cant seem to paste abput:support contents in here sorry.
could you grab the android log please? (adb logcat from the command line to get it over usb)
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.
Assignee: nobody → blassey.bugs
(Assignee)

Updated

7 years ago
Duplicate of this bug: 609897
Duplicate of this bug: 610153
Created attachment 488721 [details] [diff] [review]
logging patch

I'd like to add some logging to help figure this out.
Attachment #488721 - Flags: review?(mwu)
Duplicate of this bug: 611037
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

7 years ago
Comment on attachment 488721 [details] [diff] [review]
logging patch

r+ for the geckoevent dropping logging
Attachment #488721 - Flags: review?(mwu) → review+

Comment 16

7 years ago
Created attachment 489670 [details] [diff] [review]
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.
Assignee: blassey.bugs → mwu
Attachment #489670 - Flags: review?(blassey.bugs)
(Assignee)

Updated

7 years ago
Attachment #489670 - Flags: review?(blassey.bugs) → review+

Comment 17

7 years ago
http://hg.mozilla.org/mozilla-central/rev/815db67d28f1
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 18

7 years ago
Hm this may not have fixed all the problems. Reopening - will take another look tomorrow.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 19

7 years ago
Created attachment 492023 [details] [diff] [review]
Try very hard to catch the right intent

This one is pretty extensively tested.

Updated

7 years ago
Attachment #492023 - Flags: review?(blassey.bugs)
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()?
Attachment #492023 - Flags: review?(blassey.bugs) → review-
Created attachment 492847 [details] [diff] [review]
patch

updated to use enum
Attachment #492023 - Attachment is obsolete: true
Attachment #492847 - Flags: review?(doug.turner)
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.
(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
Created attachment 492905 [details] [diff] [review]
patch
Attachment #492847 - Attachment is obsolete: true
Attachment #492905 - Flags: review?(doug.turner)
Attachment #492847 - Flags: review?(doug.turner)
> 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?
(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 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
Attachment #492905 - Flags: review?(doug.turner) → review-
Created attachment 493094 [details] [diff] [review]
patch
Attachment #492905 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #493094 - Flags: review?(doug.turner)
Created attachment 493103 [details] [diff] [review]
patch
Assignee: mwu → blassey.bugs
Attachment #493094 - Attachment is obsolete: true
Status: REOPENED → NEW
Attachment #493103 - Flags: review?(doug.turner)
Attachment #493094 - Flags: review?(doug.turner)
Created attachment 493117 [details] [diff] [review]
patch
Attachment #493103 - Attachment is obsolete: true
Attachment #493103 - Flags: review?(doug.turner)
(Assignee)

Updated

7 years ago
Attachment #493117 - Flags: review?(doug.turner)
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?
Attachment #493117 - Flags: review?(doug.turner) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/23e28db3b63f
Status: NEW → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
This cased regression bug 614801.  I backed this out:

http://hg.mozilla.org/mozilla-central/rev/eaf1cd30f172
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 493501 [details] [diff] [review]
follow up patch

the state needs to be static to persist across activities in the same process
Attachment #493501 - Flags: review?(doug.turner)

Updated

7 years ago
Attachment #493501 - Flags: review?(doug.turner) → review+
Created attachment 493522 [details] [diff] [review]
combined patch

patch for checkin
Attachment #488721 - Attachment is obsolete: true
Attachment #489670 - Attachment is obsolete: true
Attachment #493117 - Attachment is obsolete: true
Attachment #493501 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #493522 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
verified FIXED on build:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101129 Namoroka/4.0b8pre Fennec/4.0b3pre
Status: RESOLVED → VERIFIED

Updated

7 years ago
Keywords: checkin-needed

Comment 38

7 years ago
https://litmus.mozilla.org/show_test.cgi?id=15033
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.