1186139 - e10s tp5o Regression: ContentLinkHandler.onLinkEvent() sends a IPDL::PBrowser::SendSyncMessage

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Benoit Girard (:BenWa)]

In profiles I'm seeing 8-10ms where the Content process is blocked waiting on the chrome to reply to it's sync message. This is leading to a measurable tp5o page load regression.

Comment 1

[Benoit Girard (:BenWa)]

If we want to avoid regressing tp5o with e10s we'll need to fix this.

Comment 2

[Benoit Girard (:BenWa)]

http://people.mozilla.org/~bgirard/cleopatra/?zippedProfile=http://mozilla-releng-blobs.s3.amazonaws.com/blobs/Try/sha512/5468f072507e7c17397b2bc9cf4fe3db9cf03988b9b9e44d535a0fd0e624624b07e79cc27bd88a11a7ed7a501adf99c22dea49e707f972c40c7a5719a0e88ac9&pathInZip=profile_tp5o/youtube.com/cycle_0.sps

Comment 3

[Avi Halachmi (:avih)]

(In reply to Benoit Girard (:BenWa) from comment #0)
> In profiles I'm seeing 8-10ms where the Content process is blocked waiting
> on the chrome to reply to it's sync message. This is leading to a measurable
> tp5o page load regression.

Regression compared to what?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Compared to non-e10s.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

I don't understand the code as it exists today, if there's <link rel="icon icon" href="..."> and the href is invalid, we'll try to set the favicon twice with the same invalid href. This patch just tries to set the favicon once and doesn't try again if it was valid or invalid.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8704411 [details] [diff] [review]
Patch v1

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

I wonder if there are sites that have multiple <link rel="icon" href="...">, first with some invalid ones (maybe iOS icon format?), and then with valid ones..  This would then prevent the valid ones from being tried.

Let me dig at the code history to see if there was ever a reasoning for this, or I'm just hypothesizing on a coincidence

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8704411 [details] [diff] [review]
Patch v1

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

oh nvm, the iconAdded is per <link>.  You really meant rel="icon icon", I thought that was a typo. Yeah, that looks good.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

This causes browser_bug550565.js to fail because we don't necessarily update the favicon in the parent before we fire DOMContentLoaded. I don't think this is a huge problem, but I need to update the test to not fail.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b25b436e869

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix the mochitest

Waiting for onload seems to make this work.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b7796ce9b18

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Oops

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix the mochitest

This got ugly -- we don't actually update the favicon for pushState, so I'm testing here that we don't update it (and that the favicon stays the same).

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98f7e296affd

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bc7 failures.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/03dd7a6987ea

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=98f7e296affd
12:22:37     INFO -  94 INFO TEST-START | browser/base/content/test/general/browser_bug408415.js
12:22:37     INFO -  TEST-INFO | started process screentopng
12:22:38     INFO -  TEST-INFO | screentopng: exit 0
12:22:38     INFO -  95 INFO checking window state
12:22:38     INFO -  96 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug408415.js | Correct icon before hash change. - Got null, expected chrome://mochitests/content/browser/browser/base/content/test/general/file_generic_favicon.ico
12:22:38     INFO -  Stack trace:
12:22:38     INFO -  chrome://mochikit/content/browser-test.js:test_is:965
12:22:38     INFO -  chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug408415.js:test/<:13
12:22:38     INFO -  Not taking screenshot here: see the one that was previously logged
12:22:38     INFO -  97 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug408415.js | Correct icon after hash change. - Got null, expected chrome://mochitests/content/browser/browser/base/content/test/general/file_generic_favicon.ico
12:22:38     INFO -  Stack trace:
12:22:39     INFO -  chrome://mochikit/content/browser-test.js:test_is:965
12:22:39     INFO -  chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug408415.js:test/<:15

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix this test too

I completely missed that the test that was failing on inbound wasn't browser_bug550565.js but browser_bug408415.js. It needs similar treatment.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dca990f8325

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0dca990f8325

Comment 19

[Chris Peterson [:cpeterson]]

Blake, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47? Or is this change too risky?

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8704411 [details] [diff] [review]
Patch v1

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Perf regression loading pages with favicons
[Describe test coverage new/current, TreeHerder]: There are tests for this feature on Treeherder. This patch has been on m-c for a week.
[Risks and why]: This should be low-risk. The most risky part of this patch is the timing of when the favicon on the tab is updated wrt onload. This shouldn't affect anything, though.
[String/UUID change made/needed]: n/a

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Blake, have we done a verification on Talos/tp5o that the perf regression is gone? I just want to make sure we've run perf tests + automated tests (you mentioned these in comment 20) before uplifting this to Aurora.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

mconley pointed me at https://treeherder.mozilla.org/perf.html#/graphs?timerange=2592000&series=%5Bmozilla-inbound,764594bc71db36a91df2e91a960a3a55b4fe456a,1%5D&series=%5Bmozilla-inbound,1c1fce304b8f191e35e21134106c07bb4861c0b0,1%5D&series=%5Bmozilla-inbound,f8524944919a2706fa1bcccc34af9f4ccacd84f7,1%5D&series=%5Bmozilla-inbound,eb577d5bf8c6f44fb830d194c8b5bf0d2b5bfcf5,1%5D&highlightedRevisions=0dca990f8325&zoom=1458823084265.873,1459016712704.365,207.2862362773002,357.47210988324815 which shows that if there is a gain from this patch, it's very small :-/

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8704411 [details] [diff] [review]
Patch v1

Given the low-medium risk and the fact that there isn't a significant perf improvement with this on Nightly, I'd like this one to ride the trains.