Closed Bug 542592 Opened 11 years ago Closed 11 years ago

Change how we use/store nsDocument::mLinkMap

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 6 obsolete files)

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?
Summary: Rename → Rename nsDocument::mLinkMap
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?
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.
> 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.
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...
> 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?
(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 :(
(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.
(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)
> is nsHTMLAnchorElement which would be covered by Link.

How would Link cover it for base uri changes, exactly?
(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?
(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*
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.
(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.
(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.
Summary: Rename nsDocument::mLinkMap → Change how we use/store nsDocument::mLinkMap
Attached patch v0.1 (obsolete) — Splinter Review
Not yet done, but getting there.  This will give you the general idea of what I'm going for in case anyone is interested.
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?
(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.
Attached patch v0.2 (obsolete) — Splinter Review
So, what you really want is something more like this, right?
Attachment #423904 - Attachment is obsolete: true
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].
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.
(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?
> We tell the document about it. 

Not for MathML.
As in, linking in MathML has been broken ever since we introduced nsMathMLElement.  I can point you to the relevant bugs if desired.
OK, then I guess I don't need to worry about it then.
Attached patch v0.3 (obsolete) — Splinter Review
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.
Attachment #424090 - Attachment is obsolete: true
Note to self when I get back to it next week:
revert bind/unbind changes
track when we tell the document about Links (?)
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.
(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).
> 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?
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?
(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?
> 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?
(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.
> 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.
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.
(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.
(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?
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....
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).
Attached patch v1.0 (obsolete) — Splinter Review
I've pushed this to the try server, but it looks to be it.
Attachment #424694 - Attachment is obsolete: true
Attachment #426093 - Flags: review?(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.
(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.
> 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.
ah, got it.  Will update tomorrow.
Attached patch v1.1 (obsolete) — Splinter Review
uses the batch update
Attachment #426093 - Attachment is obsolete: true
Attachment #426352 - Flags: review?(bzbarsky)
Attachment #426093 - Flags: review?(bzbarsky)
Fun fact: I did not initialize mStyledLinksCleared in the constructor in this patch.  Fixed locally.
(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.
Attached patch v1.2 (obsolete) — Splinter Review
Contains the fix to the failing test bz and I worked out.
Attachment #426352 - Attachment is obsolete: true
Attachment #426544 - Flags: review?(bzbarsky)
Attachment #426352 - Flags: review?(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.
Attachment #426544 - Flags: review?(bzbarsky) → review+
(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 :(
Attached patch v1.3Splinter Review
per review comments
Attachment #426544 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/40f19060d4c5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
Version: unspecified → Trunk
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.