1188543 - TPS doesn't score against the proper e10s tab switch event

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Benoit Girard (:BenWa)]

Similar to bug 1187451 but we need to make sure the content process is ready for the tab switch as well.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: part 2: Modify Talos

Comment 2

[Benoit Girard (:BenWa)]

Attachment: part 1: add TabSwitched event

I started looking at the event 'TabSwitchDone' first but that happens too late. It's waiting for unload for unused tabs which has a ~UNLOAD_DELAY (300ms) delay after the tab has been shown to the user.

This new event matches the times that we record to telemetry.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8640022 [details] [diff] [review]
part 1: add TabSwitched event

Review of attachment 8640022 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +3431,5 @@
>              },
>  
>              finishTabSwitch: function () {
>                if (this.requestedTab && this.getTabState(this.requestedTab) == this.STATE_LOADED) {
> +                // After this point the tab has switch from the content thread' point of view.

switch -> switched
thread' -> thread's

@@ +3433,5 @@
>              finishTabSwitch: function () {
>                if (this.requestedTab && this.getTabState(this.requestedTab) == this.STATE_LOADED) {
> +                // After this point the tab has switch from the content thread' point of view.
> +                // The changes will be visible after the next refresh driver tick + composite.
> +                let event = new CustomEvent("TabSwitched", {

I kind of like the name TabSwitchComplete better, but it doesn't really matter.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bill McCloskey (:billm) from comment #3)
> I kind of like the name TabSwitchComplete better, but it doesn't really
> matter.

Oh, I forgot we also have TabSwitchDone. I guess TabSwitched is better.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: part 1: add TabSwitched event

Comment 6

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c9822fda68ff0212b8ec811c4f4f524268274751
changeset:  c9822fda68ff0212b8ec811c4f4f524268274751
user:       Benoit Girard <[e-mail]>
date:       Wed Jul 29 13:28:05 2015 -0400
description:
Bug 1188543 - Part 1: Add TabSwitchComplete event. r=billm

Comment 7

[Benoit Girard (:BenWa)]

Attachment: part 2: Modify Talos

rebased

Comment 8

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/build/talos/rev/2beaa8352bb48879b5784d076ab980b40b3f7bf9
changeset:  2beaa8352bb48879b5784d076ab980b40b3f7bf9
user:       Benoit Girard <[e-mail]>
date:       Wed Jul 29 13:56:23 2015 -0400
description:
Bug 1188543 - TPS e10s should listen for TabSwitched event. r=blassey

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

backed out talos change:
http://hg.mozilla.org/build/talos/rev/24ecc5140689

this is because tps didn't run on try and timed out- it appears to be this change:
http://hg.mozilla.org/build/talos/rev/da09edf6d984

but backing it out has a lot of merge conflicts so we backed all of them out.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1188543 - Make sure finishTabSwitch is always on layers ready. r=blassey

Bug 1188543 - Make sure finishTabSwitch is always on layers ready. r=blassey

Comment 12

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

Comment on attachment 8646547 [details]
MozReview Request: Bug 1188543 - Make sure finishTabSwitch is always on layers ready. r=blassey

https://reviewboard.mozilla.org/r/15799/#review14085

Ship It!

Comment 13

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

https://reviewboard.mozilla.org/r/15799/#review14087

Ship It!

Comment 14

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/e9906ce9db6fe49213a23fdb4242ac915f059b9c
changeset:  e9906ce9db6fe49213a23fdb4242ac915f059b9c
user:       Benoit Girard <[e-mail]>
date:       Tue Aug 11 16:03:30 2015 -0400
description:
Bug 1188543 - Make sure finishTabSwitch is always called on layers ready. r=blassey

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Benoit Girard (:BenWa)]

Opps, still waiting on part 2

Comment 17

[Benoit Girard (:BenWa)]

Part 2 is based on top of bug 1188638, we can either wait for it to land or rebase the patch. For now I'm leaning towards wait.

Comment 18

[Benoit Girard (:BenWa)]

Attachment: part 2: Modify Talos (rebased)

Comment 19

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/try/rev/9552c93127ad

Comment 20

[Benoit Girard (:BenWa)]

Now making sure we run the test under the e10s config:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f69af64c6ba3

Comment 21

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/build/talos/rev/e970f49a2dd28c051318c577afdf84f7f0a2709f
changeset:  e970f49a2dd28c051318c577afdf84f7f0a2709f
user:       Benoit Girard <[e-mail]>
date:       Wed Jul 29 13:56:23 2015 -0400
description:
Bug 1188543 - TPS e10s should listen for TabSwitched event. r=blassey

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

this is live in production, can we close this?

Comment 23

[Gian-Carlo Pascutto [:gcp]]

(In reply to Joel Maher (:jmaher) from comment #22)
> this is live in production, can we close this?

I think this didn't actually go into production until I updated the talos config in m-c today, and received a flood of 250% talos tabswitch regressions as a result.

http://hg.mozilla.org/integration/mozilla-inbound/diff/f1b54a9c9f9b/testing/talos/talos.json

Comment 24

[Benoit Girard (:BenWa)]

That's unfortunate. But AFAIK before we were measuring the wrong thing and now we're measuring something closer to what we need. There might be further improvements we can make. But it's not like Gecko has regressed here.