Last Comment Bug 714972 - Opening links from external apps doesn't work after OOM
: Opening links from external apps doesn't work after OOM
Status: RESOLVED FIXED
[QA+]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P1 normal (vote)
: Firefox 12
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-03 14:53 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2016-07-29 14:21 PDT (History)
5 users (show)
adriant.mozilla: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
patch (2.78 KB, patch)
2012-01-03 14:53 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch (4.33 KB, patch)
2012-01-03 19:09 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch (4.67 KB, patch)
2012-01-03 19:11 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch (4.65 KB, text/plain)
2012-01-03 21:50 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details
patch to just move launch to onCreate() (4.01 KB, patch)
2012-01-03 22:27 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch to delay GeckoThread start on single core devices (2.84 KB, patch)
2012-01-03 22:28 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch to just move launch to onCreate() (4.01 KB, patch)
2012-01-04 09:43 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch to just move launch to onCreate() (4.23 KB, patch)
2012-01-04 09:45 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
patch to delay GeckoThread start on single core devices (1.64 KB, patch)
2012-01-04 10:18 PST, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review

Description User image Brad Lassey [:blassey] (use needinfo?) 2012-01-03 14:53:26 PST
Created attachment 585566 [details] [diff] [review]
patch

STR: (driven from command line)
1) adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec/.App about:fennec
2) adb shell am start -a android.intent.action.MAIN -c android.intent.category.HOME
3) adb shell /data/local/oom-fennec
4) adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec/.App about:config

Expected results:
about:config is shown

Actual results:
about:fennec shown
Comment 1 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-03 17:23:56 PST

*** This bug has been marked as a duplicate of bug 711515 ***
Comment 2 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-03 17:25:33 PST
Comment on attachment 585566 [details] [diff] [review]
patch

Is this the right patch? It doesn't seem relevant.
Comment 3 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-03 17:26:54 PST
not a duplicate
Comment 4 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-03 19:09:29 PST
Created attachment 585634 [details] [diff] [review]
patch
Comment 5 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-03 19:11:04 PST
Created attachment 585635 [details] [diff] [review]
patch
Comment 6 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-03 21:08:06 PST
Comment on attachment 585635 [details] [diff] [review]
patch

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>+    private boolean isFastDevice() {

I don't like the name of this function. I also don't like that we have this function. The use of this function is not robust enough. We are gambling.

>+        Intent intent = getIntent();

...

>+        if (intent == null)
>+            intent = getIntent();

This code should be removed. We already set the | intent | above and use it a lot. 

>+        sGeckoThread = new GeckoThread(intent, mLastUri, mLastTitle);
>+        if (isFastDevice() && checkAndSetLaunchState(LaunchState.Launching, LaunchState.Launched))
>+            sGeckoThread.start();
>+
>+        
>+

Two too many blank lines

>+        } else if (checkAndSetLaunchState(LaunchState.Launching, LaunchState.Launched)) {
>+            sGeckoThread.start();

This just doesn't feel like the right thing to do. We check to see if the device is multi-core and if it is, we start gecko in onCreate. Otherwise, we start gecko in onNewIntent. Does more than one core mean things will not be IO bound (or CPU bound) ? What if another app is running and using the second core?
Comment 7 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-03 21:50:14 PST
Created attachment 585651 [details]
patch

addresses mfinkle's comments
Comment 8 User image Mozilla RelEng Bot 2012-01-03 22:10:48 PST
Try run for 4432abf54098 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4432abf54098
Results (out of 29 total builds):
    exception: 2
    success: 20
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-4432abf54098
Comment 9 User image Mozilla RelEng Bot 2012-01-03 22:10:51 PST
Try run for 4432abf54098 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4432abf54098
Results (out of 29 total builds):
    exception: 2
    success: 20
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-4432abf54098
Comment 10 User image Mozilla RelEng Bot 2012-01-03 22:10:53 PST
Try run for 4432abf54098 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4432abf54098
Results (out of 29 total builds):
    exception: 2
    success: 20
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-4432abf54098
Comment 11 User image Mozilla RelEng Bot 2012-01-03 22:10:55 PST
Try run for 4432abf54098 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4432abf54098
Results (out of 29 total builds):
    exception: 2
    success: 20
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-4432abf54098
Comment 12 User image Mozilla RelEng Bot 2012-01-03 22:10:58 PST
Try run for 4432abf54098 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4432abf54098
Results (out of 29 total builds):
    exception: 2
    success: 20
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-4432abf54098
Comment 13 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-03 22:27:37 PST
Created attachment 585657 [details] [diff] [review]
patch to just move launch to onCreate()
Comment 14 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-03 22:28:18 PST
Created attachment 585658 [details] [diff] [review]
patch to delay GeckoThread start on single core devices
Comment 15 User image Mozilla RelEng Bot 2012-01-04 00:30:30 PST
Try run for 5478cda5bc2e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5478cda5bc2e
Results (out of 25 total builds):
    success: 19
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-5478cda5bc2e
Comment 16 User image Mozilla RelEng Bot 2012-01-04 00:30:33 PST
Try run for 5478cda5bc2e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5478cda5bc2e
Results (out of 25 total builds):
    success: 19
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-5478cda5bc2e
Comment 17 User image Mozilla RelEng Bot 2012-01-04 00:30:36 PST
Try run for 5478cda5bc2e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5478cda5bc2e
Results (out of 25 total builds):
    success: 19
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-5478cda5bc2e
Comment 18 User image Mozilla RelEng Bot 2012-01-04 00:30:39 PST
Try run for 5478cda5bc2e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5478cda5bc2e
Results (out of 25 total builds):
    success: 19
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-5478cda5bc2e
Comment 19 User image Mozilla RelEng Bot 2012-01-04 00:30:42 PST
Try run for 5478cda5bc2e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5478cda5bc2e
Results (out of 25 total builds):
    success: 19
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-5478cda5bc2e
Comment 20 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-04 09:13:23 PST
Comment on attachment 585658 [details] [diff] [review]
patch to delay GeckoThread start on single core devices

Let's move this patch to a new, specific bug.
Comment 21 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-04 09:43:43 PST
Created attachment 585788 [details] [diff] [review]
patch to just move launch to onCreate()

this patch doesn't break the debug intent
Comment 22 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-04 09:45:31 PST
Created attachment 585790 [details] [diff] [review]
patch to just move launch to onCreate()

didn't qref
Comment 23 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-04 10:18:25 PST
Created attachment 585803 [details] [diff] [review]
patch to delay GeckoThread start on single core devices

rebased to updated underlying patch
Comment 24 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-04 11:28:20 PST
Comment on attachment 585790 [details] [diff] [review]
patch to just move launch to onCreate()


>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+        sGeckoThread = new GeckoThread(intent, mLastUri, mLastTitle);
>+        if (!ACTION_DEBUG.equals(intent.getAction()) &&
>+            checkAndSetLaunchState(LaunchState.Launching, LaunchState.Launched))
>+            sGeckoThread.start();

The DEBUG intent makes us messy. I'll be glad to see some other way to do it

>-        if (checkLaunchState(LaunchState.WaitForDebugger) || launch(intent))
>+        if (checkLaunchState(LaunchState.WaitForDebugger) && intent == getIntent())

Why add the | intent == getIntent() | check? I don't see it in the old launch(..) code
Shouldn't we be checking checkLaunchState(LaunchState.Launched) instead?

Waiting for your answers before r+
Comment 25 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-04 11:33:24 PST
(In reply to Mark Finkle (:mfinkle) from comment #24)
> Comment on attachment 585790 [details] [diff] [review]
> patch to just move launch to onCreate()
> 
> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+        sGeckoThread = new GeckoThread(intent, mLastUri, mLastTitle);
> >+        if (!ACTION_DEBUG.equals(intent.getAction()) &&
> >+            checkAndSetLaunchState(LaunchState.Launching, LaunchState.Launched))
> >+            sGeckoThread.start();
> 
> The DEBUG intent makes us messy. I'll be glad to see some other way to do it
file a bug, we can discuss it there
> 
> >-        if (checkLaunchState(LaunchState.WaitForDebugger) || launch(intent))
> >+        if (checkLaunchState(LaunchState.WaitForDebugger) && intent == getIntent())
> 
> Why add the | intent == getIntent() | check? I don't see it in the old
> launch(..) code
> Shouldn't we be checking checkLaunchState(LaunchState.Launched) instead?
if the intent  passed to onNewIntent is the intent that onCreate() used, we've already passed all its data to fennec via the command line. This is essentially what the return value of launch() told us.

> Waiting for your answers before r+
Comment 26 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-04 11:38:36 PST
(In reply to Brad Lassey [:blassey] from comment #25)

> > >-        if (checkLaunchState(LaunchState.WaitForDebugger) || launch(intent))
> > >+        if (checkLaunchState(LaunchState.WaitForDebugger) && intent == getIntent())
> > 
> > Why add the | intent == getIntent() | check? I don't see it in the old
> > launch(..) code
> > Shouldn't we be checking checkLaunchState(LaunchState.Launched) instead?
> if the intent  passed to onNewIntent is the intent that onCreate() used,
> we've already passed all its data to fennec via the command line. This is
> essentially what the return value of launch() told us.

OK. I guess the checkLaunchState was just a bit more clear in that distinction
Comment 27 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-04 11:39:54 PST
Comment on attachment 585803 [details] [diff] [review]
patch to delay GeckoThread start on single core devices

patch is not relevant to this bug. obsoleting.
Comment 28 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-04 12:27:35 PST
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f84e21bcbb55
Comment 29 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-04 12:27:49 PST
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f84e21bcbb55
Comment 30 User image Alex Keybl [:akeybl] 2012-01-04 15:56:13 PST
I'll leave this in the queue until we've had a day on m-c.
Comment 31 User image Ed Morley [:emorley] 2012-01-04 17:28:25 PST
https://hg.mozilla.org/mozilla-central/rev/f84e21bcbb55
Comment 32 User image Alex Keybl [:akeybl] 2012-01-05 13:55:14 PST
Comment on attachment 585790 [details] [diff] [review]
patch to just move launch to onCreate()

[Triage Comment]
Mobile only - approved for Aurora.
Comment 33 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-06 23:31:09 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd69196df47d
Comment 34 User image Adrian Tamas (:AdrianT) 2012-09-03 07:39:12 PDT
Test case created for the scenario:

https://moztrap.mozilla.org/manage/cases/_detail/6322/

The test case has been created in the FFTs test suite.

Note You need to log in before you can comment on or make changes to this bug.