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?)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-03 14:53 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2012-09-03 07:39 PDT (History)
6 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 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 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 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 Brad Lassey [:blassey] (use needinfo?) 2012-01-03 17:26:54 PST
not a duplicate
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-01-03 19:09:29 PST
Created attachment 585634 [details] [diff] [review]
patch
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-01-03 19:11:04 PST
Created attachment 585635 [details] [diff] [review]
patch
Comment 6 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 Brad Lassey [:blassey] (use needinfo?) 2012-01-03 21:50:14 PST
Created attachment 585651 [details]
patch

addresses mfinkle's comments
Comment 8 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 Brad Lassey [:blassey] (use needinfo?) 2012-01-04 12:27:35 PST
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f84e21bcbb55
Comment 29 Brad Lassey [:blassey] (use needinfo?) 2012-01-04 12:27:49 PST
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f84e21bcbb55
Comment 30 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 Ed Morley [:emorley] 2012-01-04 17:28:25 PST
https://hg.mozilla.org/mozilla-central/rev/f84e21bcbb55
Comment 32 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 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-06 23:31:09 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd69196df47d
Comment 34 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.