787134 - mozMatchesSelector does not work for :link for elements not in a document (nor do querySelector and querySelectorAll)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Erik Arvidsson]

The following code snippet fails:

var div = document.createElement('div');
div.innerHTML = '<a href=#></a>';
var a = div.firstChild;
a.mozMatchesSelector(':link');  // Should be true

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yep, link state is determined lazily.  Bug 660959 made it less lazy, but it's still lazy.  And in particular, it happens when you insert the node into a document.

See thtread at https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.platform/6wqUct-MBZM which is what's basically needed to fix this bug.

Comment 2

[Benedict Singer]

Attachment: Patch + new testcase.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm very sorry for the review lag here; I was traveling most of Monday.  I should hopefully get to this early tomorrow.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 678062 [details] [diff] [review]
Patch + new testcase.

What prevents us from reregistering in LinkState() if !mRegistered?  Or are we assuming that this will never happen?  I'm not sure why the mRegistered set had to be moved in SetLinkState, exactly...

Apart from that, I think we should just get rid of eLinkState_Unknown.  If we have no href we should be in the NotLink state.  Instead, I think we should have a boolean that's set when we have had our href changed but haven't registered since then.  This boolean would be examined in Link::LinkState and set by ResetLinkState.  We should probably just add an argument for whether we have an href attribute to ResetLinkState instead of adding a separate method.

With this patch, <a href="http://example link"> gets the normal link styling, right?  Including the normal link mouse cursor?

Comment 5

[Boris Zbarsky [:bzbarsky]]

And in particular, after this patch IsLink() will return false in some cases that match :link.  That's probably fine, especially if you double-check the callers...

Comment 6

[Benedict Singer]

(In reply to Boris Zbarsky (:bz) from comment #5)
> And in particular, after this patch IsLink() will return false in some cases
> that match :link.  That's probably fine, especially if you double-check the
> callers...

Which cases are these? I should check that out.

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 678062 [details] [diff] [review]
> Patch + new testcase.
> 
> What prevents us from reregistering in LinkState() if !mRegistered?  Or are
> we assuming that this will never happen?  I'm not sure why the mRegistered
> set had to be moved in SetLinkState, exactly...
> 
> Apart from that, I think we should just get rid of eLinkState_Unknown.  If
> we have no href we should be in the NotLink state.  Instead, I think we
> should have a boolean that's set when we have had our href changed but
> haven't registered since then.  This boolean would be examined in
> Link::LinkState and set by ResetLinkState.  We should probably just add an
> argument for whether we have an href attribute to ResetLinkState instead of
> adding a separate method.
> 
> With this patch, <a href="http://example link"> gets the normal link
> styling, right?  Including the normal link mouse cursor?

My first try was a modification to ResetLinkState; I'll re-examine that approach. I hadn't considered getting ready of eLinkState_Unknown, but it seems possible.

Will double check that example link + the above, and try and get a new patch in this weekend.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Which cases are these? 

<a href="http://example link"> is the simple testcase for that situation.

Thanks again for looking at this!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Benedict, just to make sure: you're not waiting on me here, right?

Comment 9

[Benedict Singer]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Benedict, just to make sure: you're not waiting on me here, right?

No, sorry about the delay; I'd been trying to get another patch in for review. Now I'm back on this, and I have the approach we discussed above implemented, and I'm trying to solve a crash.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Great!  Not trying to rush you; just wanted to make sure you weren't blocked on me.  ;)

Comment 11

[Benedict Singer]

Attachment: Updated patch with no new method, Unknown state removed

Solved the crash, simplified the patch, and got it working. Added another case to the test, and put <a href="example link"> in there to verify the link styling on it. Removed the Unknown link state, and removed the new method I added previously. Ran all the mochitests, found out I broke the anchor href cache invalidation test, and fixed that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687073 [details] [diff] [review]
Updated patch with no new method, Unknown state removed

Checking HasURI() in ResetLinkState is no good, unfortunately.  It would force us to eagerly create the URI in BindToTree and UnbindFromTree, which is what a lot of this complexity is trying to prevent.  Doing that eagerly would be a serious performance regression.

The only reason we're doing that call is to compute a defaultLinkState, and the only reason we're doing that is to avoid doing unnecessary work in ResetLinkState, right?

So what work does it let us skip?

1)  The doc->ForgetLink() call.  I think we can skip that if mNeedsRegistration.
2)  The UnregisterFromHistory() call.  Again, unnecessary if mNeedsRegistration.
3)  Resetting mLinkState to defaultState.  I think we should handle this by
    having callers pass in a boolean indicating whether we have an href. 
    SetAttr can pass true, UnsetAttr can pass in false, Bind/Unbind can pass in
    the result of the relevant HasAttr() call.  Then we reset mLinkState to
    Unvisited or NotLink depending on whether we have an href.
4)  Clearing mCachedURI.  This needs to happen on every call to ResetLinkState
    when mCachedURI is not null, I believe, since bind/unbind can change the
    base URI.  So no point worrying about optimizing it away.
5)  Updating our internal state.  We only need to do this if mLinkState doesn't
    match the state we computed based on the boolean we were passed.

Does that make sense?

A few other nits:

A) I think we should set mNeedsRegistration to false as soon as we enter the "!mRegistered && mNeedsRegistration" block.  Not only because we might end up with a null hrefURI but not want to reenter this block again, but also because we don't want to keep reentering this null if !mHistory, as is the case on b2g for example.
B) No need for self->mLinkState in LinkState() when _reading_ it.  It's only needed when writing, because of the const_cast.
C) If we do what I suggest above, I believe we can remove the IsInDoc() check and setting of mLinkState in LinkState().
D) We can also remove the setting of self->mLinkState in the !hrefURI case, as well as the early return.  Just add hrefURI to the mHistory codition check, and if !hrefURI you'll just fall through to the very bottom of the method.

r- because of the perf issue, but we're getting really close!

Comment 13

[Boris Zbarsky [:bzbarsky]]

And I'm sorry again that this stuff is so fiddly.  :(

Comment 14

[Benedict Singer]

(In reply to Boris Zbarsky (:bz) from comment #12)
> Comment on attachment 687073 [details] [diff] [review]
> Updated patch with no new method, Unknown state removed
> 
> Checking HasURI() in ResetLinkState is no good, unfortunately.  It would
> force us to eagerly create the URI in BindToTree and UnbindFromTree, which
> is what a lot of this complexity is trying to prevent.  Doing that eagerly
> would be a serious performance regression.
> 
> The only reason we're doing that call is to compute a defaultLinkState, and
> the only reason we're doing that is to avoid doing unnecessary work in
> ResetLinkState, right?
> 
> So what work does it let us skip?
> 
> 1)  The doc->ForgetLink() call.  I think we can skip that if
> mNeedsRegistration.
> 2)  The UnregisterFromHistory() call.  Again, unnecessary if
> mNeedsRegistration.

Agree to both.

> 3)  Resetting mLinkState to defaultState.  I think we should handle this by
>     having callers pass in a boolean indicating whether we have an href. 
>     SetAttr can pass true, UnsetAttr can pass in false, Bind/Unbind can pass
> in
>     the result of the relevant HasAttr() call.  Then we reset mLinkState to
>     Unvisited or NotLink depending on whether we have an href.

Ah, I was trying to avoid having the caller tell the Link whether it had an href, because surely the link should know that about itself. My original solution did have callers passing in 2 booleans, but Bind/Unbind were getting it wrong some of the time, so I had the Link determine that boolean for itself. Can the Link ask mElement->HasAttr() here rather than having it passed in? Seems like it should be the same.

> 4)  Clearing mCachedURI.  This needs to happen on every call to
> ResetLinkState
>     when mCachedURI is not null, I believe, since bind/unbind can change the
>     base URI.  So no point worrying about optimizing it away.
> 5)  Updating our internal state.  We only need to do this if mLinkState
> doesn't
>     match the state we computed based on the boolean we were passed.
> 
> Does that make sense?

Agreed.

> A few other nits:
> 
> A) I think we should set mNeedsRegistration to false as soon as we enter the
> "!mRegistered && mNeedsRegistration" block.  Not only because we might end
> up with a null hrefURI but not want to reenter this block again, but also
> because we don't want to keep reentering this null if !mHistory, as is the
> case on b2g for example.

Didn't think of that case, agreed.

> B) No need for self->mLinkState in LinkState() when _reading_ it.  It's only
> needed when writing, because of the const_cast.

Yea, I saw that.

> C) If we do what I suggest above, I believe we can remove the IsInDoc()
> check and setting of mLinkState in LinkState().

This sounds correct; we should always be in Unvisited if we have an href anyway, no matter if we're in the document or not.

> D) We can also remove the setting of self->mLinkState in the !hrefURI case,
> as well as the early return.  Just add hrefURI to the mHistory codition
> check, and if !hrefURI you'll just fall through to the very bottom of the
> method.

I'll look into this.

> r- because of the perf issue, but we're getting really close!

Good! No worries that it's fiddly; I want to get this right!

Comment 15

[Boris Zbarsky [:bzbarsky]]

> but Bind/Unbind were getting it wrong some of the time

Hmm.  That's odd...

> Can the Link ask mElement->HasAttr() here rather than having it passed in?

Yes, it can.  It's a bit slower in the case when called from SetAttr (which always knows it now has an attribute), but maybe that's ok, and I agree it leads to cleaner code.  Let's do it that way for now, then maybe measure whether there's a noticeable impact on the performance of setAttribute("href") on an <a>?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Er, wait.  The problem with mElement->HasAttr() is that for different link elements the attr name is different.  So doing it that way starts getting complicated.  :(

Comment 17

[Benedict Singer]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Er, wait.  The problem with mElement->HasAttr() is that for different link
> elements the attr name is different.  So doing it that way starts getting
> complicated.  :(

Ah, that makes sense. Ok then; Set/UnsetAttr know the answer already, and Bind/Unbind can check when they need it. Which elements have attrs that aren't href? MathML/SVG?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yep.  More precisely, they have href, but they also have xlink:href.

Comment 19

[Benedict Singer]

Attachment: Patch take 3.

Ok, take 3. Dealt with the performance issue by passing in an aHasHref boolean instead of calling HasURI(); most callers know the correct value for this, but for those that don't, there's a new Link::ElementHasHref method to check the element attributes. Also incorporates most of the suggestions from the previous patch, except in ResetLinkState; replicating the correct behavior of the defaultState bail/fall-through got too complicated, so I went back to the original approach with a few fixes.

Comment 20

[Benedict Singer]

Just realized something stupid with that patch - looks like aHrefChanged isn't needed in ResetLinkState after all. I'll wait to do a new version until I know if there are any other issues though.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> replicating the correct behavior of the defaultState bail/fall-through got too complicated

Hmm.  I'd been thinking about it sort of like this:

  nsLinkState defaultState;
  if (aHasHref) {
    defaultState = eLinkState_Unvisited;
  } else {
    defaultState = eLinkState_NotLink;
  }
  if (!mNeedsRegistration && mLinkState != eLinkState_NotLink) {
    // Call ForgetLink on the document and UnregisterFromHistory
  }
  mNeedsRegistration = aHasHref;
  mCachedURI = nullptr;
  if (mLinkState == defaultState) {
    return;
  }
  mLinkState = defaultState;

Note in particular that this makes sense to call ForgetLink even if we're already in our default state but !mNeedsRegistration, which I think your patch doesn't do quite right.

In either case, defaultState doesn't need to be a member, right?

I'm not happy with ElementHasHref as written; it'll claim true for <html:a xlink:href="something">, which is not desirable.  I see why you need it; I had missed the nsDocument::RefreshLinkHrefs callsite for ResetLinkState... Perhaps what it should do is something like this:

  return mElement->HasAttr(kNameSpaceID_None, nsGkAtoms::href) ||
         (!mElement->IsHTML() && mElement->HasAttr(kNameSpaceID_XLink, nsGkAtoms::href));

?

Comment 22

[Benedict Singer]

I'll take a look at doing ResetLinkState like that - I think I've just been staring at this for too long. You're correct in any case that defaultState doesn't need to be a member.

Maybe I'm missing something (seems likely!), but doesn't Bind/Unbind not know if the element has an href either? Couldn't something be bound and then get an href set, or get an href set and then get bound?

It hadn't crossed my mind that there might be a situation with xlink:href like you described, but indeed, it does need to be changed like you suggest.

I *will* get this; just sorry it's taking so long!

Comment 23

[Boris Zbarsky [:bzbarsky]]

> but doesn't Bind/Unbind not know if the element has an href either?

That's correct, but those can just call HasAttr() directly to get that information, since they're member methods of elements already.

No worries about how long this is taking on my end!  I'm just glad you're willing to put up with all this.  ;)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, and I'm fine with Bind/Unbind calling ElementHasHref, in case that wasn't clear.  Since we need it anyway, might as well use it.

Comment 25

[Benedict Singer]

Attachment: Patch take 4.

Initially, doing the !mNeedsReg check before we set mNeedsReg seemed backwards, but after thinking about the logic became clear to me; I tried to clarify in the comment there for future readers. The attached patch appears to work, fix the issue, not introduce the performance problems, not break href cache invalidation, and has a better ElementHasHref(), as well as no aHrefChanged argument to ResetLinkState() any more. Hopefully this is it!

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 688274 [details] [diff] [review]
Patch take 4.

>+++ b/content/base/src/Link.cpp

Hmm.  Why the assertion removal in SetLinkState?  Was it triggering?

>+    if (mHistory && !!hrefURI) {

Just "if (mHistory && hrefURI) {" should be fine.

>+  // If !mNeedsReg, then either we've never registered, or we're

mNeedsRegistration, please.

>+++ b/content/base/src/Link.h
>+  virtual bool ElementHasHref() const;

I don't think this needs to be virtual.

>+++ b/content/mathml/content/src/nsMathMLElement.cpp
>+    // since there are 2 possible namspaces.

"namespaces".  And good catch on having to deal with one attr going away while the other is still present!

>+++ b/content/svg/content/src/nsSVGAElement.cpp

Hmm.  Does this not treat "href" in no namespace as a link attr?  If so, we should fix Link::ElementHasHref accordingly (test IsSVG() in there as needed).

With the above nits fixed and either the assertion restored or its removal explained, r=me.  Thank you for doing this!

Comment 27

[Benedict Singer]

(In reply to Boris Zbarsky (:bz) from comment #26)
> Comment on attachment 688274 [details] [diff] [review]
> Patch take 4.
> 
> >+++ b/content/base/src/Link.cpp
> 
> Hmm.  Why the assertion removal in SetLinkState?  Was it triggering?

I thought it was a problem since we were now more likely to be in Unvisited, but SetLinkState is only ever called with state Visited. Added back in.

> >+    if (mHistory && !!hrefURI) {
> 
> Just "if (mHistory && hrefURI) {" should be fine.

Saw this around the code, and thought it was Mozilla style. Removed.

> >+  // If !mNeedsReg, then either we've never registered, or we're
> 
> mNeedsRegistration, please.

Done.

> >+++ b/content/base/src/Link.h
> >+  virtual bool ElementHasHref() const;
> 
> I don't think this needs to be virtual.

Done. My Java training showing through.

> >+++ b/content/mathml/content/src/nsMathMLElement.cpp
> >+    // since there are 2 possible namspaces.
> 
> "namespaces".  And good catch on having to deal with one attr going away
> while the other is still present!

Done.

> >+++ b/content/svg/content/src/nsSVGAElement.cpp
> 
> Hmm.  Does this not treat "href" in no namespace as a link attr?  If so, we
> should fix Link::ElementHasHref accordingly (test IsSVG() in there as
> needed).

Done.

> With the above nits fixed and either the assertion restored or its removal
> explained, r=me.  Thank you for doing this!

New patch incoming.

Comment 28

[Benedict Singer]

Attachment: Patch final version.

Patch with final nits from review fixed.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Great!  Merged to tip and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=849a4ab8ca73

Comment 30

[Boris Zbarsky [:bzbarsky]]

Looks like this fails layout/style/test/test_visited_lying.html

This test changes the href of a link to a known-visited one but we never update the rendering to look visited.  Presumably, we're landing in ResetLinkState, detecting that the new default state is "unvisited" (which is fine), then detecting that we're already in that state, and bailing out.  What we used to do was switch into the "unknown" state then call UpdateState() on mElement, which triggered a LinkState() call, which made us re-query the history.

So just being in the default state is not enough to return early.

On the other hand, the only time it matters whether we're visited if when we have a current doc.  Maybe we should just check that in LinkState() before doing all the registering stuff and remove the early return in ResetLinkState completely?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Actually, two more test failures:

1) The layout/reftests/object/malformed-xml-with-type.html reftest is triggering assertions like so:

###!!! ASSERTION: Document knows nothing about this Link!: 'entry || mStyledLinksCleared', file ../../../../content/base/src/nsDocument.cpp, line 7236

Sounds like we may need another boolean to keep track of whether we're registered with the document.  Though we could fake it as "mRegistered || mLinkState == eLinkState_Visited", perhaps, which should be true if and only if we're registered with the document.

2) layout/style/test/test_visited_pref.html mochitest is failing.  This is the same issue as test_visited_lying.html, at first glance.

Comment 32

[Benedict Singer]

Attachment: Patch final version 2.

Fixed 2 mochitests and should fix the assertion.

Comment 33

[Boris Zbarsky [:bzbarsky]]

That last patch passes try.  So does one where the IsInDoc() check is moved up a bit so we skip the GetURI() call entirely if not IsInDoc().  Per irc discussion, going to push that one.

Comment 34

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e22af252f6e

Comment 35

[:Benjamin Peterson]

Followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0cee859ba99

Comment 36

[Boris Zbarsky [:bzbarsky]]

Benjamin, thanks!  That's what I get for making last-minute changes.  :(

Comment 37

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1e22af252f6e
https://hg.mozilla.org/mozilla-central/rev/f0cee859ba99