Opening links from external apps doesn't work after OOM

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: blassey, Assigned: blassey)

Tracking

Trunk
Firefox 12
ARM
Android
Points:
---
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

(Whiteboard: [QA+])

Attachments

(1 attachment, 8 obsolete attachments)

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
(Assignee)

Updated

6 years ago
Attachment #585566 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
Attachment #585566 - Attachment is patch: true
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 711515
Comment on attachment 585566 [details] [diff] [review]
patch

Is this the right patch? It doesn't seem relevant.
not a duplicate
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Updated

6 years ago
Status: REOPENED → NEW
Created attachment 585634 [details] [diff] [review]
patch
Assignee: nobody → blassey.bugs
Attachment #585566 - Attachment is obsolete: true
Attachment #585566 - Flags: review?(doug.turner)
Attachment #585634 - Flags: review?(doug.turner)
Created attachment 585635 [details] [diff] [review]
patch
Attachment #585634 - Attachment is obsolete: true
Attachment #585634 - Flags: review?(doug.turner)
Attachment #585635 - Flags: review?(doug.turner)
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?
Created attachment 585651 [details]
patch

addresses mfinkle's comments
Attachment #585635 - Attachment is obsolete: true
Attachment #585635 - Flags: review?(doug.turner)

Comment 8

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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
Created attachment 585657 [details] [diff] [review]
patch to just move launch to onCreate()
Attachment #585651 - Attachment is obsolete: true
Attachment #585657 - Flags: review?(doug.turner)
Created attachment 585658 [details] [diff] [review]
patch to delay GeckoThread start on single core devices
Attachment #585658 - Flags: review?(doug.turner)

Comment 15

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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 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.
Created attachment 585788 [details] [diff] [review]
patch to just move launch to onCreate()

this patch doesn't break the debug intent
Attachment #585657 - Attachment is obsolete: true
Attachment #585657 - Flags: review?(doug.turner)
Attachment #585788 - Flags: review?(mark.finkle)
Created attachment 585790 [details] [diff] [review]
patch to just move launch to onCreate()

didn't qref
Attachment #585788 - Attachment is obsolete: true
Attachment #585788 - Flags: review?(mark.finkle)
Attachment #585790 - Flags: review?(mark.finkle)
Created attachment 585803 [details] [diff] [review]
patch to delay GeckoThread start on single core devices

rebased to updated underlying patch
Attachment #585658 - Attachment is obsolete: true
Attachment #585658 - Flags: review?(doug.turner)
Attachment #585803 - Flags: review?(doug.turner)
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+
(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+
(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
Attachment #585790 - Flags: review?(mark.finkle) → review+
Comment on attachment 585803 [details] [diff] [review]
patch to delay GeckoThread start on single core devices

patch is not relevant to this bug. obsoleting.
Attachment #585803 - Attachment is obsolete: true
Attachment #585803 - Flags: review?(doug.turner)
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f84e21bcbb55
Whiteboard: [inbound][fennec-aurora]
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f84e21bcbb55
Priority: -- → P1
(Assignee)

Updated

6 years ago
Attachment #585790 - Flags: approval-mozilla-aurora?
I'll leave this in the queue until we've had a day on m-c.
https://hg.mozilla.org/mozilla-central/rev/f84e21bcbb55
Status: NEW → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12

Updated

6 years ago
Flags: in-litmus?(fennec)
Whiteboard: [inbound][fennec-aurora] → [inbound][fennec-aurora][QA+]
Comment on attachment 585790 [details] [diff] [review]
patch to just move launch to onCreate()

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #585790 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

6 years ago
tracking-fennec: --- → 11+
status-firefox11: --- → affected
status-firefox12: --- → fixed
Whiteboard: [inbound][fennec-aurora][QA+] → [inbound][QA+]
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd69196df47d
status-firefox11: affected → fixed
Whiteboard: [inbound][QA+] → [QA+]
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.
Flags: in-litmus?(fennec) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.