714972 - Opening links from external apps doesn't work after OOM

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

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

Attachment: 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 2

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

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?)]

not a duplicate

Comment 4

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

Attachment: patch

Comment 5

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

Attachment: patch

Comment 6

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

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?)]

Attachment: patch

addresses mfinkle's comments

Comment 8

[Mozilla RelEng Bot]

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/[e-mail]-4432abf54098

Comment 9

[Mozilla RelEng Bot]

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/[e-mail]-4432abf54098

Comment 10

[Mozilla RelEng Bot]

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/[e-mail]-4432abf54098

Comment 11

[Mozilla RelEng Bot]

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/[e-mail]-4432abf54098

Comment 12

[Mozilla RelEng Bot]

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/[e-mail]-4432abf54098

Comment 13

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

Attachment: patch to just move launch to onCreate()

Comment 14

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

Attachment: patch to delay GeckoThread start on single core devices

Comment 15

[Mozilla RelEng Bot]

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/[e-mail]-5478cda5bc2e

Comment 16

[Mozilla RelEng Bot]

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/[e-mail]-5478cda5bc2e

Comment 17

[Mozilla RelEng Bot]

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/[e-mail]-5478cda5bc2e

Comment 18

[Mozilla RelEng Bot]

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/[e-mail]-5478cda5bc2e

Comment 19

[Mozilla RelEng Bot]

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/[e-mail]-5478cda5bc2e

Comment 20

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

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?)]

Attachment: patch to just move launch to onCreate()

this patch doesn't break the debug intent

Comment 22

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

Attachment: patch to just move launch to onCreate()

didn't qref

Comment 23

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

Attachment: patch to delay GeckoThread start on single core devices

rebased to updated underlying patch

Comment 24

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

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?)]

(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?)]

(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?)]

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?)]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f84e21bcbb55

Comment 29

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

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f84e21bcbb55

Comment 30

[Alex Keybl [:akeybl]]

I'll leave this in the queue until we've had a day on m-c.

Comment 31

[Ed Morley [:emorley]]

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

Comment 32

[Alex Keybl [:akeybl]]

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?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd69196df47d

Comment 34

[Adrian Tamas (:AdrianT)]

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.