Bug 197229

Summary: Visited link hash should be computed only once
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, darin, ews-watchlist, ggaren, rniwa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
wip
none
patch
none
patch
achristensen: review+, commit-queue: commit-queue-
patch
commit-queue: commit-queue-
patch none

Description Antti Koivisto 2019-04-24 03:09:18 PDT
Don't allow URL mutations to affect the hash afterwards.
Comment 1 Antti Koivisto 2019-04-24 03:10:44 PDT
Created attachment 368116 [details]
wip
Comment 2 EWS Watchlist 2019-04-24 04:02:53 PDT
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.
Comment 3 Antti Koivisto 2019-04-24 04:05:36 PDT
<rdar://problem/48438924>
Comment 4 Antti Koivisto 2019-04-24 06:04:28 PDT
Created attachment 368121 [details]
patch
Comment 5 Antti Koivisto 2019-04-24 07:35:20 PDT
Created attachment 368122 [details]
patch
Comment 6 Alex Christensen 2019-04-24 08:17:11 PDT
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?
Comment 7 Antti Koivisto 2019-04-24 09:10:55 PDT
> Would mutable Optional<const SharedStringHash> work?

Doesn't compile.
Comment 8 WebKit Commit Bot 2019-04-24 09:12:44 PDT
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
Comment 9 Antti Koivisto 2019-04-25 00:36:20 PDT
Created attachment 368219 [details]
patch
Comment 10 WebKit Commit Bot 2019-04-25 02:05:56 PDT
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
Comment 11 Antti Koivisto 2019-04-25 02:16:38 PDT
Created attachment 368223 [details]
patch
Comment 12 WebKit Commit Bot 2019-04-25 02:55:44 PDT
Comment on attachment 368223 [details]
patch

Clearing flags on attachment: 368223

Committed r244642: <https://trac.webkit.org/changeset/244642>
Comment 13 WebKit Commit Bot 2019-04-25 02:55:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2019-04-25 09:09:49 PDT
Is this change OK from a user experience point of view?
Comment 15 Geoffrey Garen 2019-04-25 09:35:33 PDT
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.
Comment 16 Antti Koivisto 2019-04-26 01:19:26 PDT
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).