542592 - Change how we use/store nsDocument::mLinkMap

Status: RESOLVED FIXED

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

Description

[Shawn Wilsher :sdwilsh]

From bug 461199 comment 199:
> This looks ok; is there a followup bug to reevaluate mLinkMap in light of this
> change?
We still need it (for base URI changes), but bz says we should rename it and update comments.  No good name is coming to mind though - thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

I wasn't just talking about renaming.  Once we land bug 461199, what is it used for?  Base URI changes, yes, but what happens with the map on a base URI change?

Comment 2

[Shawn Wilsher :sdwilsh]

The consumers that are left are:
1) Cycle collection traversal
2) nsDocument::AddStyleRelevantLink (where it is added)
3) nsDocument::ForgetLink (where it is removed)
4) Setup and Initialization of the mLinkMap
5) nsDocument::RefreshLinkHrefs

(5) is the only that that really matters.  That is called in two places:
a) nsDocument::SetDocumentURI
b) nsDocument::SetBaseURI

I don't think we can get rid of it, but I'm open to suggestions.  We should probably make this store Link instead, and then we can get rid of nsIContent::DropCachedHref.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I don't think we can get rid of it

Right, but we also don't need to have it in the form of a uri-to-node hashtable.  Just a hash set of nsIContent or Link pointers would do fine, right?  Or anything else that lets us walk the whole list; since we don't need random access an array would do just as well, except for the higher add/remove costs.

Note that we don't use the link map for DropCachedHref.

Comment 4

[Shawn Wilsher :sdwilsh]

I think we can actually get rid of DropCachedHrefsRecursive (and nsIContent::DropCachedHref).  RefreshLinkStateVisitor::Visit would then take a mozilla::dom::Link, which would then just call Link::ResetLinkState, which would drop the cached href.

So, do we just want to store it in an nsTArray of Link *?  Do we need to hold a strong reference to it since Link tells the document to forget about it anyway (in ResetLinkState):
  // Tell the document to forget about this link.
  nsIDocument *doc = content->GetCurrentDoc();
  if (doc) {
    doc->ForgetLink(content);
  }

I suppose Link should also be adding it to the document?  Right now, with bug 461199 applied, I don't have anything calling AddStyleRelevantLink.  I would expect tests to be failing though...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I think we can actually get rid of DropCachedHrefsRecursive 

Can we?  Is everything with a cached href in the link map?  It wasn't last I checked.

> I don't have anything calling AddStyleRelevantLink.  I would expect tests to be
> failing though...

I would too.  Why aren't they?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Can we?  Is everything with a cached href in the link map?  It wasn't last I
> checked.
The only class that implements nsIContent::DropCachedHref (other than the default from nsIContent) is nsHTMLAnchorElement which would be covered by Link.
 
> I would too.  Why aren't they?
Because the test that covers it (test_bug209275.xhtml) is also hit by bug 534526 :(

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> I think we can actually get rid of DropCachedHrefsRecursive (and
> nsIContent::DropCachedHref).  RefreshLinkStateVisitor::Visit would then take a
> mozilla::dom::Link, which would then just call Link::ResetLinkState, which
> would drop the cached href.
This also means we can get rid of nsIContent::SetLinkState.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> This also means we can get rid of nsIContent::SetLinkState.
(and we could get rid of bug 461199 part 22)

Comment 9

[Boris Zbarsky [:bzbarsky]]

> is nsHTMLAnchorElement which would be covered by Link.

How would Link cover it for base uri changes, exactly?

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> How would Link cover it for base uri changes, exactly?
The key here is that nsDocument::RefreshLinkHrefs uses RefreshLinkStateTraverser, which calls nsUint32ToContentHashEntry::VisitContent, which calls RefreshLinkStateVisitor::Visit which is where we'd call Link::ResetLinkState (per comment 4).  Right?

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Because the test that covers it (test_bug209275.xhtml) is also hit by bug
> 534526 :(
And that only seems to actually impact the SVG bits, not the HTML bits.  Making it so we get added to the link map makes the HTML bits pass.  I'll fix the test and upload that new part in the bug.  Also introduced a new crash by adding to the link map that I'm debugging now.  *sigh*

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  The whole point is that on base uri change we just need to ResetLinkState and trigger a ContentStatesChanged on all the Links that are style relevant.  So we just need to keep track of the style-relevant links.

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to comment #12)
> Ah, yes.  The whole point is that on base uri change we just need to
> ResetLinkState and trigger a ContentStatesChanged on all the Links that are
> style relevant.  So we just need to keep track of the style-relevant links.

Yes, I think that's right.  The code could be better documented to indicate that it assumes the link map contains all the style-relevant links.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Ah, yes.  The whole point is that on base uri change we just need to
> ResetLinkState and trigger a ContentStatesChanged on all the Links that are
> style relevant.  So we just need to keep track of the style-relevant links.
Fixing that stuff in bug 461199 first (since it's very broken there).  Then we'll tackle this problem.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

Not yet done, but getting there.  This will give you the general idea of what I'm going for in case anyone is interested.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Calling ContentStatesChanged while iterating worries me.  The old code had those explicit comments about not doing that, right?

This looks like it could make removals really slow.  Maybe a hashset instead of an array?

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Calling ContentStatesChanged while iterating worries me.  The old code had
> those explicit comments about not doing that, right?
The old code says we can't call it because it might modify the hash table, but since we just track all links (and don't drop some out in special cases like before) we can iterate and notify at the same time.

> This looks like it could make removals really slow.  Maybe a hashset instead of
> an array?
nsTHashtable<nsPtrHashKey<Link> > it is then.

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v0.2

So, what you really want is something more like this, right?

Comment 19

[Shawn Wilsher :sdwilsh]

Hrm - the changes to ForgetLink and adding a link to take Link instead of nsIContent might not be doable, which kinda sucks.  This basically breaks any XLink handling code.  We could, I guess, make nsMathMLElement inherit Link so it can do this stuff, or I leave it as nsIContent, which is not really what I want to do.  See also attachment 399812 [details] [diff] [review].

Comment 20

[Boris Zbarsky [:bzbarsky]]

If we're sure that the hashset can't be modified during the enumeration, then that patch looks good.

I'm not sure what comment 19 is about.  MathML doesn't do xlink right now anyway; it'd need to inherit Link to be a link, no?  In fact, we have no xlink anything that's not actually a Link at this point, I thought.

Comment 21

[Shawn Wilsher :sdwilsh]

(In reply to comment #20)
> If we're sure that the hashset can't be modified during the enumeration, then
> that patch looks good.
The hash table will assert like mad if we start modifying it, so it'll be easy to spot if this changes.  Right now, we don't modify it.

> I'm not sure what comment 19 is about.  MathML doesn't do xlink right now
> anyway; it'd need to inherit Link to be a link, no?  In fact, we have no xlink
> anything that's not actually a Link at this point, I thought.
Well, before bug 461199 it should work (I think).  We tell the document about it.  If we change the nsIDocument APIs for handling of links to take Link instead of nsIContent, we'll break that.  Are we OK with that?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> We tell the document about it. 

Not for MathML.

Comment 23

[Boris Zbarsky [:bzbarsky]]

As in, linking in MathML has been broken ever since we introduced nsMathMLElement.  I can point you to the relevant bugs if desired.

Comment 24

[Shawn Wilsher :sdwilsh]

OK, then I guess I don't need to worry about it then.

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: v0.3

This isn't fully tested (need to catch train), but this may be version 1.0.  It also makes ResetLinkState notify the document per e-mails with bz.

Comment 26

[Shawn Wilsher :sdwilsh]

Note to self when I get back to it next week:
revert bind/unbind changes
track when we tell the document about Links (?)

Comment 27

[Boris Zbarsky [:bzbarsky]]

You need update batches around the non-nsDocument callers of ResetLinkState (and docs on the function saying the caller must have an update batch), right?

Alternately, I suppose we could send a begin/end inside ResetLinkState and just nest those inside the nsDocument batch.

Do we ResetLinkState from bind/unbind?  If so, we should make sure to NOT notify from those calls, imo.

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> You need update batches around the non-nsDocument callers of ResetLinkState
> (and docs on the function saying the caller must have an update batch), right?
I don't think most places call it in a batch, but maybe we should add some documentation about it.

> Alternately, I suppose we could send a begin/end inside ResetLinkState and just
> nest those inside the nsDocument batch.
I don't follow this comment - please explain?

> Do we ResetLinkState from bind/unbind?  If so, we should make sure to NOT
> notify from those calls, imo.
Ah, we do now, and this might explain why I'm having assertion issues (I added assertions when we add a remove a link in the document to make sure we didn't already know/not know about it before).

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I don't follow this comment - please explain?

The current contract for ContentStatesChanged is that it must be preceded by a BeginUpdate call and followed by an EndUpdate call.  The ResetLinkState code in this patch doesn't have such calls.  The nsDocument caller of ResetLinkState has an update batch around the whole operation.  The other callers would need to do their own Begin/EndUpdates.

Sounds like ResetLinkState should probably take an aNotify boolean?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Oh, one other thing... On the base URI change, if we go from unvisited to visited, what triggers a new history lookup with this patch?

Comment 31

[Shawn Wilsher :sdwilsh]

(In reply to comment #30)
> Oh, one other thing... On the base URI change, if we go from unvisited to
> visited, what triggers a new history lookup with this patch?
We end up calling ResetLinkState, which clears our state, so when we next call IntrinsicState, we'll kick off a lookup.  We have tests for this that pass right now (in bug 461199) right?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> so when we next call IntrinsicState

Right, but when will that be?

> We have tests for this that pass right now

Without the patch attached here, yes.  With this patch, do they?

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> > so when we next call IntrinsicState
> Right, but when will that be?
I guess you are implying they won't be...  I guess that's because we notify?

> Without the patch attached here, yes.  With this patch, do they?
I'm about 90% sure the patches to fix the tests is later in my queue.  I'll know for sure on Monday.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> I guess you are implying they won't be...  I guess that's because we notify?

In the cases in which we notify, the ensuing style reresolve will call IntrinsicState().  But if we change the state and don't notify, no one will ever know anything needs to happen.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Oh, one more thing.  I'd fully expect the relevant IntrinsicState() check to happen under the ContentStatesChanged call.  That call needs to happen after we set mLinkState; basically at the very end of ResetLinkState.

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #29)
> The current contract for ContentStatesChanged is that it must be preceded by a
> BeginUpdate call and followed by an EndUpdate call.  The ResetLinkState code in
> this patch doesn't have such calls.  The nsDocument caller of ResetLinkState
> has an update batch around the whole operation.  The other callers would need
> to do their own Begin/EndUpdates.
So, for what it's worth, it would be awesome if a few things could be done to make this clearer:
1) Add documentation that says this is the case since right now it doesn't:
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDocument.h#665
2) Add an assertion in the implementation such that makes sure all consumers follow this contract in debug builds so developers catch this when they write the code.

Not sure where this should be done (new bug?), but please make it happen for new contributor's (to this code) sake.

Comment 37

[Shawn Wilsher :sdwilsh]

(In reply to comment #29)
> Sounds like ResetLinkState should probably take an aNotify boolean?
In what situations would we need to notify and what ones would we not want to notify?  I'm guessing the only time we do not want to notify in nsDocument (what this bug is changing), right?

Comment 38

[Boris Zbarsky [:bzbarsky]]

Generally speaking, we want to notify whenever the node has a frame that might have its styles change and not otherwise, for performance reasons.  In particular, we do not want to notify on attr sets from the parser (attr sets with aNotify false) and do not want to notify on bind/unbind.  We also don't want to notify if someone else will also notify, of course....

Comment 39

[Shawn Wilsher :sdwilsh]

So, I believe all cases where we notify we are already in a batch (all call sites are touched in this patch), so I think we don't need to add the batching in ResetLinkState.  The only case I'm not certain about is from SetAttr and UnsetAttr.

I believe I've addressed all your concerns in the upcoming patch (compiling now to make sure it builds properly).

Comment 40

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

I've pushed this to the try server, but it looks to be it.

Comment 41

[Boris Zbarsky [:bzbarsky]]

> So, I believe all cases where we notify we are already in a batch

I don't see where, for the SetAttr/UnsetAttr callsites... Note that batches are tagged with the _kind_ of update; content updates and state updates need separate batches.

Comment 42

[Shawn Wilsher :sdwilsh]

(In reply to comment #41)
> I don't see where, for the SetAttr/UnsetAttr callsites... Note that batches are
> tagged with the _kind_ of update; content updates and state updates need
> separate batches.
Those would be the two cases I'm not certain about per comment 39.  So I need to do an update batch in ResetLinkState when we notify, correct?  Can we do nested updates?  If not, I need to rethink how I do things in nsDocument.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> Those would be the two cases I'm not certain about per comment 39.

Ah, indeed. I misread that comment.

Those two callsites are not, generally, in a content-state update batch.

> So I need to do an update batch in ResetLinkState when we notify, correct?

That seems like the simplest approach to me, yes.  Note that MOZ_AUTO_DOC_UPDATE should do things correctly for you (as in, takes an aNotify argument and doesn't do a batch if it's false).

> Can we do nested updates? 

Yes.  See comment 27 paragraph 2.

Comment 44

[Shawn Wilsher :sdwilsh]

ah, got it.  Will update tomorrow.

Comment 45

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

uses the batch update

Comment 46

[Shawn Wilsher :sdwilsh]

Fun fact: I did not initialize mStyledLinksCleared in the constructor in this patch.  Fixed locally.

Comment 47

[Shawn Wilsher :sdwilsh]

(In reply to comment #46)
> Fun fact: I did not initialize mStyledLinksCleared in the constructor in this
> patch.  Fixed locally.
Scratch that, don't have to:
  // NOTE! nsDocument::operator new() zeroes out all members, so don't
  // bother initializing members to 0.

Comment 48

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Contains the fix to the failing test bz and I worked out.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426544 [details] [diff] [review]
v1.2

>+++ b/content/base/src/Link.cpp
>+Link::ResetLinkState(bool aNotify)
>+  // We need to notify in case our state changed and we were told to notify.

How about:

  If aNotify is true, notify both of the visited-related states.  We have
  to do that, because we might be racing with a response from history and
  hence need to make sure that we get restyled whether we were visited or
  not before.  In particular, we need to make sure that our LinkState() is
  called so that we'll start a new history query as needed.

>+++ b/content/base/src/nsDocument.cpp
>+#include "mozilla/dom/Link.h"
>+using namespace mozilla::dom;

I would really prefer at least the include, and maybe the using, at the top of the file.

>+namespace {

Might be clearer to just make this static.

>+  nsTArray<Link*> linksToNotify(mStyledLinks.Count());

Is it worth making this an nsAutoTArray with some size like 100 or so?  Probaby doesn't matter much...

>+++ b/content/base/src/nsDocument.h
>+  // An array of styled links.
>+  nsTHashtable<nsPtrHashKey<mozilla::dom::Link> > mStyledLinks;

Fix comment to match reality?

>+  // Indicates if mStyledLinks was cleared or not.  This is used to track state

s/if/whether/

r=bzbarsky with those nits.

Comment 50

[Shawn Wilsher :sdwilsh]

(In reply to comment #49)
> Is it worth making this an nsAutoTArray with some size like 100 or so?  Probaby
> doesn't matter much...
Likely not since it will only do one allocation to get the right size anyway.  Also, nsAutoTArray has no constructor to resize :(

Comment 51

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

per review comments

Comment 52

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/40f19060d4c5