Closed Bug 1888756 Opened 4 months ago Closed 25 days ago

Text-Fragments: Implement User Activation

Categories

(Core :: DOM: Navigation, task)

task

Tracking

()

RESOLVED FIXED
129 Branch
Tracking Status
firefox129 --- fixed

People

(Reporter: jjaschke, Assigned: jjaschke)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

(Keywords: perf-alert)

Crash Data

Attachments

(7 files, 1 obsolete file)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

This bug tracks the implementation of the User Activation behavior of Text Fragments.

Blocks: 1898630

This commit is a prerequisite for part 5 of this patch set
and introduces the text directive user activation flag
as well as the necessary boilerplate to transport the value
through process boundaries in nsDocShellLoadState and LoadInfo.

There is no changed behavior in this commit.

This commit is a prerequisite for part 5 of this patch set.

This flag is set in the docshell, based on user gesture activation.
This is not necessarily sufficient,
as the text fragments spec bases the value of
this flag also on User Involvement, which is not
implemented in Gecko yet.

The flag is then plumbed through the parent process
(DocumentLoadListener::CreateDocumentLoadInfo())
to the document, where it can be consumed.
This includes client-side redirects as described in [0].

There is no changed behavior in this commit.

[0] https://wicg.github.io/scroll-to-text-fragment/#restricting-the-text-fragment

This commit is a prerequisite for part 5 of this patch set.

The idea is to keep the information whether the last
load done by the doc shell was a same-document navigation.
The value is reset for every navigation, therefore it
should always be correct.

There is no changed behavior in this commit.

This commit is a prerequisite for part 5 of this patch set
and does not introduce any changes in behavior.

This commit implements the rules given in the spec [0]
to determine if a text directive is allowed to be scrolled to.

This includes checking and consuming the text directive user activation
which was introduced in the previous parts of this patch set.

Additionally, a number of checks given in [1] are implemented.
The newly added method is injected into our scroll-to-fragment
algorithms in Document::ScrollToRef() and nsDocShell::ScrollToAnchor().

[0] https://wicg.github.io/scroll-to-text-fragment/#restricting-the-text-fragment
[1] https://wicg.github.io/scroll-to-text-fragment/#check-if-a-text-directive-can-be-scrolled

## non-html-documents.html

Gecko does not fire an onload event for JSON files,
which causes the test to time out.
Therefore, the test for JSON has been moved
to the end of the file, so that the other tests
run before the test times out.

Note: The rules implemented in part 5 of this
patch set prevent JSON files from being scrolled to;
Failing this test does not imply incorrect behavior.

## iframe-scroll.sub.html

This test opens a same/cross origin iframe with a text fragment.
According to the test file, the iframe should
scroll to the text fragment in both cases.
Additionally, the same-origin case should update
the parent document's scroll position for the same-origin case.

In the same-origin case, the current Gecko implementation
scrolls inside of the iframe, but does not bubble
the scroll position to the parent document.
In the cross-origin case, the iframe does not scroll,
because it is a cross-origin non-top-level navigation.

Currently, user activation is not transported correctly
if fission is disabled.
This makes a lot of tests fail now, as no user activation
prevents scrolling and most tests rely on scrolling.

Attachment #9395315 - Attachment is obsolete: true
See Also: → 1901064

Here is a current try run.

There are two issues currently:

  1. User Activation is not moved across process boundaries correctly if Fission is disabled. This would currently prevent text directives from being scrolled to on Android. I've filed Bug 1901064. Once this is fixed, the test expectations added in part 7 of this patchset can be reverted.
  2. There are issues regarding consumption of the user gesture activation in combination with the popup blocker. If the popup blocker is enabled, it consumes the user activation, before the text directive user activation is created based off of its value. When running WPTs, the popup blocker is disabled, hence the tests pass. However, in real-life scenarios, the text directive will typically not be scrolled to if the page is navigated to using window.open(). Edgar is filing a follow-up bug for and looking into this.

Both issues do not leak sensitive information -- text directives that are actually allowed to be scrolled to are not being scrolled to, not vice versa. Therefore these issues should not block landing this feature.

Attachment #9405915 - Attachment description: Bug 1888756, part 6 - Text Fragments: Changed some WPTs and expected results. r=#dom-core → Bug 1888756, part 6 - Text Fragments: Changed order of tests in non-html-documents.html. r=#dom-core
Pushed by jjaschke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0eb8b13b0717
part 1 - Text Fragments: Introduce `textDirectiveUserActivation` flag. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/04180e2fb99d
part 2 - Text Fragments: Implement setting the `textDirectiveUserActivation` flag in the document loading algorithm. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/400a8ff57ae4
part 3 - Text Fragments: Add a flag in the `DocShell` to keep the information if a load was same-document. r=farre,dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/efff3e5e95c2
part 4 - Text Fragments: Add more logging to parsing/stripping the fragment directive. r=dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/a84d886d8753
part 5 - Text Fragments: Implemented user activation. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/1458ad53d229
part 6 - Text Fragments: Changed order of tests in non-html-documents.html. r=dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/85022f2e4129
part 7 - Text Fragments: Tentatively allow tests to fail when fission is not enabled. r=edgar,dom-core
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46838 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
Regressions: 1903877
Crash Signature: [@ mozilla::net::DocumentLoadListener::Open ]

Backed out for potentially causing Session History crashes.
Backout link: https://hg.mozilla.org/mozilla-central/rev/deea4a904a0d3d546ab359aeca91a4bb6dc02a77

Status: RESOLVED → REOPENED
Flags: needinfo?(jjaschke)
Resolution: FIXED → ---
Target Milestone: 129 Branch → ---
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46859 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
Attachment #9405912 - Attachment description: Bug 1888756, part 3 - Text Fragments: Add a flag in the `DocShell` to keep the information if a load was same-document. r=farre!,#dom-core → Bug 1888756, part 3 - Text Fragments: Preserve information if a load is a same-document load in `nsILoadInfo`. r=edgar!,#dom-core
Blocks: 1904773
Pushed by jjaschke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c706ebb5cbf8
part 1 - Text Fragments: Introduce `textDirectiveUserActivation` flag. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/3b60fbb80f67
part 2 - Text Fragments: Implement setting the `textDirectiveUserActivation` flag in the document loading algorithm. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/142591a5a921
part 3 - Text Fragments: Preserve information if a load is a same-document load in `nsILoadInfo`. r=farre,dom-core,necko-reviewers,kershaw,peterv
https://hg.mozilla.org/integration/autoland/rev/e1915755c9ca
part 4 - Text Fragments: Add more logging to parsing/stripping the fragment directive. r=dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/339436073065
part 5 - Text Fragments: Implemented user activation. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/11957961b0fa
part 6 - Text Fragments: Changed order of tests in non-html-documents.html. r=dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/662204fa8621
part 7 - Text Fragments: Tentatively allow tests to fail when fission is not enabled. r=edgar,dom-core
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46909 for changes under testing/web-platform/tests

The new version fixes the session history crashes and adjusts the test results mentioned in bug 1904773.

Flags: needinfo?(jjaschke)
Upstream PR merged by moz-wptsync-bot

Hello !
The following alert was generated by the backout . Do you think this bug or one of the following Bug 1888756 , Bug 1901064, Bug 1888756 is responsible for generating the alert ?

Perfherder has detected a browsertime performance change from push 5f6f41a83f0ea8d64264cf04f49a5940f9c8705b.

Regressions:

Ratio Test Platform Options Absolute values (old vs new) Performance Profiles
22% wikipedia fcp android-hw-a51-11-0-aarch64-shippable-qr warm webrender 69.60 -> 85.21 Before/After
11% speedometer3 TodoMVC-Preact-Complex-DOM/Adding100Items/Sync macosx1400-64-shippable-qr fission webrender 0.70 -> 0.78 Before/After
8% speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Sync macosx1400-64-shippable-qr fission webrender 1.51 -> 1.62 Before/After
7% speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Sync windows10-64-shippable-qr fission webrender 3.21 -> 3.43 Before/After
6% speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Sync windows10-64-nightlyasrelease-qr fission webrender 3.17 -> 3.37 Before/After
5% speedometer3 TodoMVC-Preact-Complex-DOM/Adding100Items/total macosx1400-64-shippable-qr fission webrender 5.19 -> 5.44 Before/After
4% speedometer3 TodoMVC-Svelte-Complex-DOM/CompletingAllItems/total windows10-64-shippable-qr fission webrender 7.37 -> 7.69 Before/After
4% speedometer3 Charts-chartjs/Show tooltip/Sync macosx1400-64-shippable-qr fission webrender 0.08 -> 0.08 Before/After
4% speedometer3 TodoMVC-Svelte-Complex-DOM/CompletingAllItems/Async windows10-64-shippable-qr fission webrender 5.97 -> 6.18 Before/After
3% speedometer3 TodoMVC-Preact-Complex-DOM/total macosx1400-64-shippable-qr fission webrender 11.59 -> 11.93 Before/After
2% speedometer3 TodoMVC-Preact-Complex-DOM/DeletingAllItems/Async macosx1400-64-shippable-qr fission webrender 1.74 -> 1.78 Before/After

Improvements:

Ratio Test Platform Options Absolute values (old vs new) Performance Profiles
17% fandom largestContentfulPaint linux1804-64-shippable-qr fission warm webrender 47.46 -> 39.44 Before/After
9% speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Async windows10-64-nightlyasrelease-qr fission webrender 9.35 -> 8.52 Before/After
8% wikia FirstVisualChange linux1804-64-shippable-qr cold fission webrender 330.15 -> 305.37 Before/After
7% speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Async windows10-64-shippable-qr fission webrender 9.36 -> 8.67 Before/After
7% wikia largestContentfulPaint linux1804-64-shippable-qr fission warm webrender 193.05 -> 180.32 Before/After
... ... ... ... ... ...
2% speedometer3 Charts-observable-plot/Stacked by 6/Async macosx1400-64-shippable-qr fission webrender 2.38 -> 2.33 Before/After

As author of one of the patches included in that push, we need your help to address this regression.
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 899

For more information on performance sheriffing please see our FAQ.

Flags: needinfo?(jjaschke)

Hi Florin,

I can take a look, but I highly doubt that the backout of this bug, or the bug itself, can cause a perf regression. The underlying feature is prefed off, so the changes in these patches should be (almost) a noop.

Given that the patches have landed again today, I would suggest re-running the tests to see if this still persists. WDYT?

Flags: needinfo?(jjaschke) → needinfo?(fbilt)
Regressions: 1905074

(In reply to Jan Jaeschke [:jjaschke] from comment #23)

Hi Florin,

I can take a look, but I highly doubt that the backout of this bug, or the bug itself, can cause a perf regression. The underlying feature is prefed off, so the changes in these patches should be (almost) a noop.

Given that the patches have landed again today, I would suggest re-running the tests to see if this still persists. WDYT?

The regression was recorded on this revision : https://treeherder.mozilla.org/jobs?repo=autoland&revision=5f6f41a83f0ea8d64264cf04f49a5940f9c8705b&group_state=expanded&selectedTaskRun=LHnMakbESkuPnRwRmX-4eg.0

Also you can see the graph:
https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&replicates=0&selected=5042290,43548387&series=autoland,5042290,1,13&timerange=1209600&zoom=1718863319466,1719053273250,1.3119688284156954,1.7668973643782215

In this revision multiple bugs were backed out, including your bugs. (Bug 1901799, Bug 1897271, Bug 1900863, Bug 1902364, Bug 1902671, Bug 1902086, Bug 1901771, Bug 1902003, Bug 1902359, Bug 1888756, Bug 1901064 Bug 1860915 Bug 1888756)
I left a 'need info' request for each of the bug owners, hoping to find out which of the bug caused the alert.

Flags: needinfo?(fbilt)

could it be that making text-fragment parsing preffed off actually improved performance and the backout ended up having worse performance? In Bug 1901799#c13 :fbilt mentioned it is the backout that had the worse performance. I see that backout performance profiles introduced extra mozilla::dom::FragmentDirective::FindTextFragmentsInDocument calls, so backing out the preffing off actually made performance worse? Can you confirm if this is what might have happened?

Flags: needinfo?(jjaschke)

Ah, sorry for the confusion. The value of the pref has not changed. FragmentDirective::FindTextFragmentsInDocument() should always be a noop as long as the pref is turned off. Could you show me the exact profile you've been looking into?

Flags: needinfo?(jjaschke) → needinfo?(fkilic)

Ah I see. This one is the one with our patches (before the backout) and this one is after the backout. I see calls to nsHtml5ExecutorReflusher increased due to mozilla::dom::Document::ScrollToRef in the backout. Everything else looks somewhat similar. This is my first time dealing with perfherder, so if my finding is totally wrong, I'm sorry hahaha.

Flags: needinfo?(fkilic)

Oof. Nice find. I'll take back everything I said :)

Okay, here's the explanation:
FindTextFragmentsInDocument() did flush frames unconditionally before this bug was applied. Applying part 4 of the patches added an early return in that function before flushing. Therefore, applying the patch improved performance, and backing it out restored the slowness. Now it all makes sense, sorry. I was not aware that I introduced the early return here, I thought it was there all along.

But, at the end of the day, I think no further action is necessary because the change has now landed (again). Sounds good?

Flags: needinfo?(fkilic)
Flags: needinfo?(fbilt)

Yep, sounds good to me. Thank you!

Flags: needinfo?(fkilic)

Yes, sounds good. Thank you :)

Flags: needinfo?(fbilt)
Keywords: perf-alert
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: