Bug 197229 – Visited link hash should be computed only once

• Component: DOM
• Status: RESOLVED FIXED

[Antti Koivisto]

Don't allow URL mutations to affect the hash afterwards.

[Antti Koivisto]

Created attachment 368116 [details]
wip

[EWS Watchlist]

Attachment 368116 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGAElement.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/dom/VisitedLinkState.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

<rdar://problem/48438924>

[Antti Koivisto]

Created attachment 368121 [details]
patch

[Antti Koivisto]

Created attachment 368122 [details]
patch

[Alex Christensen]

Comment on attachment 368122 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368122&action=review

> Source/WebCore/html/HTMLAnchorElement.h:119
> +    // This is computed only once and must not be affected by subsequent URL changes.
> +    mutable Optional<SharedStringHash> m_storedVisitedLinkHash;

Would mutable Optional<const SharedStringHash> work?

[Antti Koivisto]

> Would mutable Optional<const SharedStringHash> work?

Doesn't compile.

[WebKit Commit Bot]

Comment on attachment 368122 [details]
patch

Rejecting attachment 368122 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 368122, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Traceback (most recent call last):
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: https://webkit-queues.webkit.org/results/11983672

[Antti Koivisto]

Created attachment 368219 [details]
patch

[WebKit Commit Bot]

Comment on attachment 368219 [details]
patch

Rejecting attachment 368219 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 368219, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Traceback (most recent call last):
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: https://webkit-queues.webkit.org/results/11993303

[Antti Koivisto]

Created attachment 368223 [details]
patch

[WebKit Commit Bot]

Comment on attachment 368223 [details]
patch

Clearing flags on attachment: 368223

Committed r244642: <https://trac.webkit.org/changeset/244642>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Is this change OK from a user experience point of view?

[Geoffrey Garen]

We believe the user experience won't change because we believe that non-attacker pages don't reassign the href value after rendering a link element. If we're right, this policy is the clearest, and maybe even the easiest to specify in the future.

The most likely failure mode we've imagined is a page where all the links turn purple because they start out as href="" and then JavaScript fills in non-empty href values later. ("" is considered visited because it resolves to the current page.)

If we do find pages with links that start out with href="a.html" and change to href="b.html", alternatives we've discussed are: (a) throttle style changes triggered by href changes using a timer; (b) unconditionally force a full style recalc for all href changes, even if nothing has changed.

[Antti Koivisto]

Note that href mutations did not reliably produce correct visited styling before this change either, because we didn't actually invalidate the style. It only worked if some other change triggered a style recalc (like in the test case).