Closed Bug 1556000 Opened 3 months ago Closed 3 months ago

Remove the Content Window Size Telemetry

Categories

(Core :: Window Management, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: tjr, Assigned: timhuang)

References

Details

Attachments

(2 files)

No description provided.
Priority: -- → P3
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ef75e158b232
Remove the expired telemetry probe of the content window size. r=Gijs

(In reply to Tim Huang[:timhuang] from comment #3)

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=316a461e803ea550d737ef0ff460f38d88652bb5

FWIW, the failures that got this backed out are in that trypush, too...

Please bump the count at https://searchfox.org/mozilla-central/source/browser/base/content/test/performance/browser_startup_syncIPC.js#62-65 to 3 instead of 2 and then reland.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(tihuang)

Sorry for that I didn't catch this failure in the first place.

I tried to bump the count to 3, however, try still shows the same failure. [1]

Maybe we need to whitelist 'PCompositorBridge::Msg_MapAndNotifyChildCreated' for the stage 'before becoming idle'. But it's still unclear to me why this happened. Is it because of removing this telemetry changes the timing of the startup?

Could you give insights here, Florian?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dec0f7e72b5000a7fdf25810eb65193512cdbf5e&selectedJob=250404529

Flags: needinfo?(tihuang) → needinfo?(florian)

(In reply to Tim Huang[:timhuang] from comment #6)

Maybe we need to whitelist 'PCompositorBridge::Msg_MapAndNotifyChildCreated' for the stage 'before becoming idle'. But it's still unclear to me why this happened. Is it because of removing this telemetry changes the timing of the startup?

Could you give insights here, Florian?

The "becoming idle" thing here was supposed to mean "once we are fully done with startup", but it's poorly implemented with a hack at https://searchfox.org/mozilla-central/rev/ee806041c6f76cc33aa3c9869107ca87cb3de371/browser/components/tests/startupRecorder.js#150

This seems to not fully cover the end of startup, especially on Windows. The sync IPC the test is reporting in the failures you experienced are related to starting content processes. I had already noticed that sometimes on Windows startup recorder stops before we have created all the content processes that are usually created during startup.

A side effect of your patch is to make the NewTabPagePreloading.maybeCreatePreloadedBrowser(window); line execute slightly earlier.

In the short term, don't worry about this and just whitelist the newly reported sync IPC.

The correct long term solution is to replace the hack in startupRecorder.js with something that waits for a promise that would be resolved once we are done with all the idle tasks running during late startup from BrowserGlue.jsm and browser.js. This will require significant work as we'll need to adjust whitelists in a bunch of startup tests; it's fine to keep this for a follow-up bug.

Flags: needinfo?(florian)
Attachment #9069954 - Attachment description: Bug 1556000 - Remove the expired telemetry probe of the content window size. r?gijs! → Bug 1556000 - Part 1: Remove the expired telemetry probe of the content window size. r?gijs!

We need to whistlist 'PCompositorBridge::Msg_MapAndNotifyChildCreated'
sync IPC in the stage 'before becoming idle' for Windows platform since
removing the content window telemetry probe causing this sync IPC been
sent earlier than before. And the IPC recorder sometimes doesn't fully
cover the stage in Windows platform. So, we will get this sync IPC in
the stage and we need to whitelist it.

Depends on D33788

(In reply to Tim Huang[:timhuang] from comment #8)

Created attachment 9071218 [details]
Bug 1556000 - Part 2: Whitelist sync IPC 'PCompositorBridge::Msg_MapAndNotifyChildCreated' for the test 'browser_startup_syncIPC.js'. r?florian!

Is this enough to make the test green on try? I thought in comment 6 you were saying you need some more whitelisting. I would like to see a green try run with the TV (verify) jobs before r+'ing. Thanks!

Flags: needinfo?(tihuang)

No, this is not enough. I still get failure here [1].

We need to whitelist both 'PCompositorBridge::Msg_MapAndNotifyChildCreated' and 'PCompositorBridge::Msg_NotifyChildCreated' in order to get a green on try.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3b9bbdc083f05c379722c4e13fc5dc8df7e37b1&selectedJob=251124342

Flags: needinfo?(tihuang)
Attachment #9071218 - Attachment description: Bug 1556000 - Part 2: Whitelist sync IPC 'PCompositorBridge::Msg_MapAndNotifyChildCreated' for the test 'browser_startup_syncIPC.js'. r?florian! → Bug 1556000 - Part 2: Whitelist sync IPCs 'PCompositorBridge::Msg_MapAndNotifyChildCreated' and 'PCompositorBridge::Msg_NotifyChildCreated' for the test 'browser_startup_syncIPC.js'. r?florian!
Pushed by tihuang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5fed2a257c4
Part 1: Remove the expired telemetry probe of the content window size. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/e16ada7fe052
Part 2: Whitelist sync IPCs 'PCompositorBridge::Msg_MapAndNotifyChildCreated' and 'PCompositorBridge::Msg_NotifyChildCreated' for the test 'browser_startup_syncIPC.js'. r=florian
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.