880259 - Firefox should use GeckoView

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: patch

We need the patch on bug 876689 before this lands, but other than that things work fine.

Comment 1

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

I'd like to see a Try server run. I also think we should hold off on landing this until we merge for Fx25, just to give us as much time as possible. We're not in a rush to get GeckoView into Fx24 and I'd rather not take the added risk.

Sound OK?

Comment 2

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

(In reply to Mark Finkle (:mfinkle) from comment #1)
> I'd like to see a Try server run. I also think we should hold off on landing
> this until we merge for Fx25, just to give us as much time as possible.
> We're not in a rush to get GeckoView into Fx24 and I'd rather not take the
> added risk.
> 
> Sound OK?

yup

Comment 3

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

Attachment: patch

adding sLock to GeckoThread is the thing that fixed the test failures from the original patch. It was on my to-do list to fix that since Kats pointed it out last week.

Along the way of diagnosing this I did some of the other things on my to-do list such as moving GeckoThread initialization into GeckoThread. That is a good thing, but I don't think it is strictly required for this to land.

Here's a try run:

https://tbpl.mozilla.org/?tree=Try&rev=23d09d8c6b2a

Comment 4

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

Comment on attachment 773392 [details] [diff] [review]
patch

The Try run had a few oranges. I retriggered. Code seems sane.

Comment 5

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/855854e05453 - the try run had quite a few oranges, and the retriggers remained orange, but it was actually quite a bit more orange on inbound, mixing in reftest failures and unexpected passes and talos timeouts and failures to initialize and shutdown crashes at mozilla::layers::CompositorParent::ResumeComposition() with the same timeouts the try run showed.

Comment 6

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

Attachment: use_geckoview.patch

now that bug 894313 has the gecko thread ownership model sorted out, this works locally (running test via adb). Pushed to try to confirm.

https://tbpl.mozilla.org/?tree=Try&rev=c16cf271aca6

The doinit is a bit of a crutch. There are two remaining issues to get sorted out before I'm really happy with this. The first is moving the ownership of GeckoApp.mProfile to GeckoProfile (something like get/setSelectedProfile()). The other is to sort out the start up timeline for starting the gecko thread. Right now it is a bit racy, so I prefer it to be just handled by GeckoApp for Firefox until it that gets sorted.

Comment 7

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

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> Created attachment 785184 [details] [diff] [review]
> use_geckoview.patch
> 
> now that bug 894313 has the gecko thread ownership model sorted out, this
> works locally (running test via adb). Pushed to try to confirm.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=c16cf271aca6
> 
> The doinit is a bit of a crutch.

I hope this means you plan to remove it someday soon?

> There are two remaining issues to get
> sorted out before I'm really happy with this. The first is moving the
> ownership of GeckoApp.mProfile to GeckoProfile (something like
> get/setSelectedProfile()).

Wes opened bug 902060 for this

> The other is to sort out the start up timeline
> for starting the gecko thread. Right now it is a bit racy, so I prefer it to
> be just handled by GeckoApp for Firefox until it that gets sorted.

Is this the doinit part?

Comment 8

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

Comment on attachment 785184 [details] [diff] [review]
use_geckoview.patch

The doinit part of this is not acceptable for GeckoView as a user library. It's OK for Fennec  to use internally while we figure out the proper fix.

The Try run has some orange. I re-triggered to see if the orange is persistent. Check that out before landing.

Comment 9

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

(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> > Created attachment 785184 [details] [diff] [review]
> > use_geckoview.patch

> > The other is to sort out the start up timeline
> > for starting the gecko thread. Right now it is a bit racy, so I prefer it to
> > be just handled by GeckoApp for Firefox until it that gets sorted.
> 
> Is this the doinit part?

The doinit part fixes works around both the profile stuff and the racy start

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 785184 [details] [diff] [review]
> use_geckoview.patch
> 
> The doinit part of this is not acceptable for GeckoView as a user library.
> It's OK for Fennec  to use internally while we figure out the proper fix.
> 
> The Try run has some orange. I re-triggered to see if the orange is
> persistent. Check that out before landing.

I already retriggered the oranges and got greens.

Comment 10

[Geoff Brown [:gbrown]]

Were you expecting any talos regressions?

Regression: Mozilla-Inbound - Robocop Checkerboarding Real User Benchmark - Android 4.0.4 - 22.2% increase
----------------------------------------------------------------------------------------------------------
    Previous: avg 4.946 stddev 0.373 of 12 runs up to revision aad7f060dadb
    New     : avg 6.046 stddev 0.394 of 12 runs since revision 5de4d20f6cfd
    Change  : +1.100 (22.2% / z=2.950)
    Graph   : http://mzl.la/17BhVO1

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=aad7f060dadb&tochange=5de4d20f6cfd

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/5de4d20f6cfd
    : Brad Lassey <[e-mail]> - bug 880259 - Firefox should use GeckoView r=mfinkle
    : http://bugzilla.mozilla.org/show_bug.cgi?id=880259

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=880259 - Firefox should use GeckoView


------------------------------


Regression: Mozilla-Inbound - Robocop Checkerboarding Real User Benchmark - Android 2.2 (Native) - 18.7% increase
-----------------------------------------------------------------------------------------------------------------
    Previous: avg 6.106 stddev 0.437 of 12 runs up to revision aad7f060dadb
    New     : avg 7.249 stddev 0.227 of 12 runs since revision f538bf3eafc3
    Change  : +1.143 (18.7% / z=2.613)
    Graph   : http://mzl.la/17BhX8D

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=aad7f060dadb&tochange=f538bf3eafc3

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/5de4d20f6cfd
    : Brad Lassey <[e-mail]> - bug 880259 - Firefox should use GeckoView r=mfinkle
    : http://bugzilla.mozilla.org/show_bug.cgi?id=880259

Comment 11

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

(In reply to Geoff Brown [:gbrown] from comment #10)
> Were you expecting any talos regressions?
> 
no

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing frequent Android reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba04505ce73

https://tbpl.mozilla.org/php/getParsedLog.php?id=26354197&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26355136&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26353831&tree=Mozilla-Inbound

Comment 13

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

Attachment: use_geckoview.patch

solid green this time

https://tbpl.mozilla.org/?tree=Try&rev=c95798c546fc

Comment 14

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

Comment on attachment 789331 [details] [diff] [review]
use_geckoview.patch

Try looked fine and the only difference I can see is that you moved the doInit check sooner in the process. We still want to remove that in the future.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25a567f1e260
https://hg.mozilla.org/mozilla-central/rev/000bb98adba6