Closed Bug 1361618 Opened 7 years ago Closed 6 years ago

Remove unnecessary sync calls to FlushPendingLinkUpdates()

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed
firefox60 --- fixed

People

(Reporter: bzbarsky, Assigned: mrbkap)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
On the other hand, maybe even the Gecko version should go away; see bug 1361709.
Depends on: 1361709
Seems related to :visited work in bug 1328509, I can investigate this.  It's possible this is needed for :visited tests to pass anyway...?
If I'm right in bug 1361709 then it's not needed for those tests to pass...
Assignee: nobody → jryans
Priority: -- → P2
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=af8bbab1f363d6219343ca20a9523f7a57ba95bf

I removed the two calls to `FlushPendingLinkUpdates` in nsCSSFrameConstructor and GeckoRestyleManager, and tests look good, but it's a bit tricky to have confidence about what's happening here.

:bz and I looked through what the pending link code is attempting to do:

* When a new anchor is bound in `BindToTree` we do:
  1. Link::ResetLinkState(false, Link::ElementHasHref());
    * Set link's mLinkState to default (either unvisited or not link)
    * Set element's link mState bits to default (either unvisited or no link state)
  2. doc->RegisterPendingLinkUpdate(this);
    * Schedules idle dispatch to run `FlushPendingLinkUpdates` within 1 sec
* In `FlushPendingLinkUpdates`:
  * For each pending link, call element->UpdateLinkState(link->LinkState());
    1. Register link for async history update to get potential future visited state
    2. mLinkState is still unvisited / not link until we hear from history, so element state is unchanged

Thus, there seems to be no need to call `FlushPendingLinkUpdates` outside of `BindToTree`, since visited state is always applied async anyway (so it doesn't work as an optimization to avoid restyling if visited, since that will trigger later).
Summary: stylo: Stylo needs some equivalent of the FlushPendingLinkUpdates() in ElementRestyler::Restyle → Remove unnecessary sync calls to FlushPendingLinkUpdates()
Comment on attachment 8882658 [details]
Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates.

https://reviewboard.mozilla.org/r/153736/#review159094

::: commit-message-cc903:21
(Diff revision 1)
> +    1. Register link for async history update to get potential future visited
> +       state
> +    2. mLinkState is still unvisited / not link until we hear from history, so
> +       element state is unchanged
> +
> +Thus, there seems to be no need to call `FlushPendingLinkUpdates` outside of

I don't understand this comment.
You explicitly remove the synchronous handling.
Comment on attachment 8882658 [details]
Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates.

I don't think I should review this. Either bz or dbaron should take a look.
(I can't convince myself immediately why this change is ok.)
Attachment #8882658 - Flags: review?(bugs)
Comment on attachment 8882658 [details]
Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates.

https://reviewboard.mozilla.org/r/153736/#review159094

> I don't understand this comment.
> You explicitly remove the synchronous handling.

I agree it's confusing!  The tricky part here is that although `FlushPendingLinkUpdates` feels synchronous, it only ever sets the default link state.  It calls `LinkState()`[1] which returns that default value, and also registers a history listener for a potential _async_ visited update.

[1]: http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/dom/base/Link.cpp#190
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8882658 [details]
> Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates.
> 
> I don't think I should review this. Either bz or dbaron should take a look.
> (I can't convince myself immediately why this change is ok.)

I'll try :dbaron, as he may have more context about this.  :bz and I looked at this code during the SF work week, and we thought it seemed okay to remove as I've done here, but it's hard to feel confident about it!
Comment on attachment 8882658 [details]
Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates.

https://reviewboard.mozilla.org/r/153736/#review161010

It's probably been too long since I've really looked at this code for me to say all that much useful here -- other than noting that link coloring becoming async was something that happened *while* I was developing (and most of the way through the work) the original patches for the :visited privacy attack, so it's entirely possible (if this goes back to that landing -- didn't check if it did) that it was the result of the code working differently when I actually wrote it, prior to landing.

Other than that, I'm largely trusting your explanation here (which seems reasonable) and the fact that you and bz looked into it.

Sorry for not getting to this earlier -- I'd been thinking I was actually going to really understand how this all worked again, but that's probably not really necessary to do this review.
Attachment #8882658 - Flags: review?(dbaron) → review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9a7b90cf8a63
Remove unnecessary calls to FlushPendingLinkUpdates. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/9a7b90cf8a63
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/2fba314d7de7
Backed out changeset 9a7b90cf8a63 for frequently failing accessible/tests/browser/states/browser_test_link.js (bug 1385498). r=backout a=backout
Backed out for frequently failing accessible/tests/browser/states/browser_test_link.js (bug 1385498):

https://hg.mozilla.org/mozilla-central/rev/2fba314d7de77ad8ab693a2ea0112c0cda5dd564

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=119118241&repo=autoland
[task 2017-07-28T22:39:48.443877Z] 22:39:48     INFO - Entering test bound 
[task 2017-07-28T22:39:48.446603Z] 22:39:48     INFO - Buffered messages logged at 22:39:02
[task 2017-07-28T22:39:48.449340Z] 22:39:48     INFO - TEST-PASS | accessible/tests/browser/states/browser_test_link.js | Accessible document present. - 
[task 2017-07-28T22:39:48.452595Z] 22:39:48     INFO - TEST-PASS | accessible/tests/browser/states/browser_test_link.js | state bits should not be present in ID ['a@id="link_traversed" node', address: http://www.example.com/, role: link, name: 'example.com', address: [xpconnect wrapped nsIAccessible]]! - 
[task 2017-07-28T22:39:48.461912Z] 22:39:48     INFO - Buffered messages finished
[task 2017-07-28T22:39:48.464412Z] 22:39:48     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/states/browser_test_link.js | Test timed out - 
[task 2017-07-28T22:39:48.466754Z] 22:39:48     INFO - GECKO(1104) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2017-07-28T22:39:48.470899Z] 22:39:48     INFO - GECKO(1104) | MEMORY STAT | vsize 863MB | residentFast 232MB | heapAllocated 92MB
[task 2017-07-28T22:39:48.472347Z] 22:39:48     INFO - TEST-OK | accessible/tests/browser/states/browser_test_link.js | took 45330ms
[task 2017-07-28T22:39:48.481589Z] 22:39:48     INFO - checking window state
[task 2017-07-28T22:39:48.483780Z] 22:39:48     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-07-28T22:39:48.485640Z] 22:39:48     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/states/browser_test_link.js | Found a browser window after previous test timed out -
Status: RESOLVED → REOPENED
Flags: needinfo?(jryans)
Resolution: FIXED → ---
Clearing ni? for now, but hoping to try again here when time allows.
Flags: needinfo?(jryans)
I think it's unlikely that I will return to this soon.
Assignee: jryans → nobody
I'd like to re-land this and see if we're still failing browser_test_link.js -- I've pushed to try twice and tried running the test countless times locally without ever seeing it fail. I'm hoping (optimistically, I admit) that other fixes have fixed whatever it was that made this patch bounce the first time. At the very least, pushing this again should allow me to inspect new logs and possibly push logging patches to figure out what's going on.
Assignee: nobody → mrbkap
Attachment #8882658 - Attachment is obsolete: true
David, this patch is equivalent to the one that you reviewed 7 months ago, I needed a new mozreview that I could re-open and interact with.
Comment on attachment 8950428 [details]
Bug 1361618 - Remove unnecessary calls to FlushPendingLinkUpdates.

https://reviewboard.mozilla.org/r/219674/#review225430
Attachment #8950428 - Flags: review?(dbaron) → review+
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37b91f33d4f3
Remove unnecessary calls to FlushPendingLinkUpdates. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/37b91f33d4f3
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
See Also: → 1479069
You need to log in before you can comment on or make changes to this bug.