Closed Bug 714972 Opened 8 years ago Closed 8 years ago

Opening links from external apps doesn't work after OOM

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
firefox12 --- fixed
fennec 11+ ---

People

(Reporter: blassey, Assigned: blassey)

Details

(Whiteboard: [QA+])

Attachments

(1 file, 8 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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
Attachment #585566 - Flags: review?(doug.turner)
Attachment #585566 - Attachment is patch: true
Status: NEW → RESOLVED
Closed: 8 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 → ---
Status: REOPENED → NEW
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #585566 - Attachment is obsolete: true
Attachment #585566 - Flags: review?(doug.turner)
Attachment #585634 - Flags: review?(doug.turner)
Attached patch patch (obsolete) — Splinter Review
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?
Attached file patch (obsolete) —
addresses mfinkle's comments
Attachment #585635 - Attachment is obsolete: true
Attachment #585635 - Flags: review?(doug.turner)
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
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
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
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
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
Attachment #585651 - Attachment is obsolete: true
Attachment #585657 - Flags: review?(doug.turner)
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
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
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
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
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.
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)
didn't qref
Attachment #585788 - Attachment is obsolete: true
Attachment #585788 - Flags: review?(mark.finkle)
Attachment #585790 - Flags: review?(mark.finkle)
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)
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
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
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+
tracking-fennec: --- → 11+
Whiteboard: [inbound][fennec-aurora][QA+] → [inbound][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.