Closed Bug 1186139 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
e10s + ---
firefox42 --- affected
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: BenWa, Assigned: mrbkap)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
If we want to avoid regressing tp5o with e10s we'll need to fix this.
tracking-e10s: --- → ?
Assignee: nobody → mrbkap
Priority: -- → P1
(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?
Compared to non-e10s.
Attached patch Patch v1Splinter Review
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.
Attachment #8704411 - Flags: review?(felipc)
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 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.
Attachment #8704411 - Flags: review?(felipc) → review+
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.
Attached patch Fix the mochitest (obsolete) — Splinter Review
Waiting for onload seems to make this work.
Attachment #8726969 - Flags: review?(felipc)
Attachment #8726969 - Flags: review?(felipc) → review+
Oops
Flags: needinfo?(felipc)
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).
Attachment #8729758 - Flags: review?(felipc)
Attachment #8726969 - Attachment is obsolete: true
Attachment #8729758 - Flags: review?(felipc) → review+
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
Flags: needinfo?(mrbkap)
I completely missed that the test that was failing on inbound wasn't browser_bug550565.js but browser_bug408415.js. It needs similar treatment.
Attachment #8734543 - Flags: review?(felipc)
Attachment #8734543 - Flags: review?(felipc) → review+
Flags: needinfo?(mrbkap)
https://hg.mozilla.org/mozilla-central/rev/0dca990f8325
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Blake, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47? Or is this change too risky?
Flags: needinfo?(mrbkap)
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
Flags: needinfo?(mrbkap)
Attachment #8704411 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(mrbkap)
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.
Attachment #8704411 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.