Fix the Document::mSubDocuments usage in Document::ReportUseCounters
Categories
(Core :: DOM: Core & HTML, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox83 | --- | fixed |
People
(Reporter: hiro, Assigned: heycam)
References
Details
Attachments
(5 files)
Document::mSubDocuments has sub documents in the same process. So if we need to aggregate the numbers per the top level content document, we need to somehow collect the information via IPC calls or such.
Reporter | ||
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
Right now, use counter reporting is done under Document::Destroy()
. In ReportUseCounters
, we call into each document in mSubDocuments
to ask them to (a) report their own document-level use counters, and (b) propagate the page-level use counters up to the top-level content document. Since the top-level content document is in the middle of being destroyed, we can't just do this asynchronously.
Maybe we can change to a model of subdocuments pro-actively reporting their use counters up to the top-level content document (e.g. off a background task when a use counter is set).
Reporter | ||
Comment 3•4 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #2)
Right now, use counter reporting is done under
Document::Destroy()
. InReportUseCounters
, we call into each document inmSubDocuments
to ask them to (a) report their own document-level use counters, and (b) propagate the page-level use counters up to the top-level content document. Since the top-level content document is in the middle of being destroyed, we can't just do this asynchronously.Maybe we can change to a model of subdocuments pro-actively reporting their use counters up to the top-level content document (e.g. off a background task when a use counter is set).
Or maybe subdocuments send them to the parent process and the top-level content document also send them to the parent process, then in the parent process we can sum up them and report the telemetry data there?
Assignee | ||
Comment 4•4 years ago
|
||
Yes that would work too, and be less IPC traffic overall.
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
Planning to do something like this:
- Leave reporting of document use counters to be done by each document under
Document::Destroy()
, as we do now. - At some point early on in a document that would report use counters but which is not a top-level content document, we determine which top-level content document we'll propagate our use counters to as page use counters. We record this document using its (outer?) window ID. We then send a message to the parent process telling it that we will be reporting page use counters to count towards the specified window ID at some point. When the parent process receives this message, it increments a counter for the number of sub-document use counters it'll need to wait for.
- Under
Destroy()
for any document that would report use counters, we send a message to the parent process with our final use counter data and the window ID. The parent process accumulates the use counter data. If we've received the expected number of messages containing use counter data, we report them via telemetry.
We might also want to keep the current "loop over sub-documents and ask them to propagate use counters" part of ReportUseCounters
, replacing that with browsing content descendant traversal and sending them a message, to avoid making the final telemetry wait for documents to be cycle collected.
One downside is that if a content process crashes, then any document in it that would be the beneficiary of propagated page use counters wouldn't get any page use counters reported at all, since the parent process would still be waiting for further messages. We could work around that by having the top-level content document, under its Destroy
, send a message to the parent process saying that it expects all use counter messages to be sent soon, and the parent could kick off a timer to report the telemetry if it doesn't receive the remaining data in time. Not clear if it's worth it though.
Emilio (as you were last to tweak the way we accumulate and report use counter data, in bug 1578700) do you forsee any problems with this?
Comment 6•4 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #5)
Planning to do something like this:
- Leave reporting of document use counters to be done by each document under
Document::Destroy()
, as we do now.- At some point early on in a document that would report use counters but which is not a top-level content document, we determine which top-level content document we'll propagate our use counters to as page use counters. We record this document using its (outer?) window ID.
I think you need the (browsing-context-id, top-level-inner-window-id)
pair... The outer window can be shared across top level documents.
Emilio (as you were last to tweak the way we accumulate and report use counter data, in bug 1578700) do you forsee any problems with this?
Other than that, no, this sounds like a sensible solution.
Comment 7•4 years ago
|
||
Fission M6b because we want use counters to work for our Fission Nightly experiment.
Comment 8•4 years ago
|
||
Based on comment 5, it looks like Cameron will be working on this.
Comment 9•4 years ago
|
||
Fission dependency already indirectly present via the audit meta bug.
Comment hidden (obsolete) |
Assignee | ||
Comment 11•4 years ago
|
||
We always have one.
Assignee | ||
Comment 12•4 years ago
|
||
Adding this to be used when the bit set needs to be sent across IPC.
Assignee | ||
Comment 13•4 years ago
|
||
Assignee | ||
Comment 14•4 years ago
|
||
This changes the way we deal with page use counters so that we can
handle out of process iframes.
Currently, when a parent document is being destroyed, we poke into all
of the sub-documents to merge their use counters into the parent's page
use counters, which we then report via Telemetry. With Fission enabled,
the sub-documents may be out of process. We can't simply turn these
into async IPC calls, since the parent process will be destroyed
shortly.
So instead, each document during its initialization identifies which
ancestor document it will contribute its page use counters to, and
stores its WindowContext Id to identify that ancestor. A message is
sent to the parent process to notify it that page use counter data will
be sent at some later point. That later point is when the document is
being destroyed. It doesn't matter if the ancestor document has already
been destroyed at this point, since all we need is its WindowContext Id
to uniquely identify it. Once the parent process has received all of
the use counters it expects to accumulate to a given WindowContext Id,
it reports them via Telemetry.
Reporting of document use counters remains unchanged and is done by each
document in their content process.
This patch fixes a few other issues:
-
We no longer report use counters for initial about:blank pages, which
have been having the effect of lowering the document use counter
percentages, since CONTENT_DOCUMENTS_DESTROYED was being incremented
for them. -
We no longer report use counters for documents created by addons.
-
Added proper MOZ_LOG logging for use counters instead of printfs.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Updated•4 years ago
|
Comment 18•4 years ago
|
||
Please provide an estimate date for the patch in the Whiteboard field as M6b is nearing completion.
Comment hidden (obsolete) |
Assignee | ||
Comment 20•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 21•4 years ago
|
||
Comment 22•4 years ago
|
||
Comment 23•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e606925d0468
https://hg.mozilla.org/mozilla-central/rev/34138216874d
https://hg.mozilla.org/mozilla-central/rev/7d0e7bdf9d6e
https://hg.mozilla.org/mozilla-central/rev/ef64855313bd
https://hg.mozilla.org/mozilla-central/rev/d7dac872984d
Updated•1 year ago
|
Description
•