485553 - [FIX]Anchor-mutation functions should store the URI object, not the string

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Now that bug 481335 is fixed, we should store the URI object, not the string in the anchor mutations functions.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Or rather, store both.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

The change this bug is about is the first part of the SetHrefToURI change.

The nsHTMLAnchorElement change fixes a bug I found while looking at this code, and the test tests for that bug.  In particular, since GetHref() uses the cached URI, and the mutators mutate it, we were getting equality when the attr set came from SetHrefToURI, even though the URI had changed.  The right equality check is no check at all, I think; the chance that SetAttr is happening with the same value as we already had is low, and I have no problem dropping the URI cache in that case.

That said, we don't get dropped from the link map correctly in that case, because that uses our cached URI (which has changed by then).  The consequence is the node being kept alive too long, I think.  So the other change is to drop us from the link map when getting ready to edit the URI.  But since we might edit it in a way that doesn't actually call into the style system to reresolve (e.g. don't change the href), we need to manually readd in SetHrefToURI.

The test covers the various correctness aspects of this (everything mentioned above except link map bloat and the caching we were not doing).

Note that the caching call I'm adding doesn't affect the correctness bugs this patch fixes; those are present even without it.

If we think that readding all links to the link map on anchor.* mutations is bad, we could alternately clone the URI in GetHrefURIForEditing.  Then we could probably restore the equality check in SetAttr, as well as getting rid of the link map munging I added.  That does involve a separate allocation, though.  Not sure which is faster...  Cloning is certainly cleaner.

Thoughts?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: With cloning

I did some measurements, and this is no slower than the other patch.  It _is_ simpler, though.

About half the time, sadly, is spent doing the IsVisited() check when constructing the restyle data for checking whether we have attribute dependent style...

There's also wasted time because said attribute dependent style check forces parsing of the URI, and happens before we've managed to set the new URI object on the nsAttrValue in a lot of cases.  I can do a followup but to try to fix this, if you'd like.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Come to think of which, we're atomically changing both the link state and the attr value; I bet we can write testcases for dynamic changes that fail to restyle correctly in this case...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: microbenchmark I used

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 369732 [details] [diff] [review]
With cloning

From: Boris Zbarsky <[e-mail]>
Bug 485553.  Fix issue with mutating anchor uri properties and visited state, and cache the resulting URI when the URI is mutated instead of just reparsing it later.  r+sr=sicking

Did sicking already review this, or something? :)

This patch looks good to me, the only thing that came to mind when reading through it was a naming thing...

+  // As above, but removes us from the link map using the URI before we edit it
+  void GetHrefURIForEditing(nsIURI** aURI);

... which was here. Would this be better named something like GetHrefURIToMutate(), or somesuch. At first glance I though editing had something to do with our editor, which is of course not the case. I don't care strongly here, just figured I'd throw it out there for you to consider as well.

r+sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Did sicking already review this, or something? :)

Er, no.  I originally planned to ask him for review, but then decided that he's lagging enough as it is.  ;)

> Would this be better named something like GetHrefURIToMutate()

Yeah, makes sense.  I'll do that.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/9bdd54c5f99f with that change.