698237 - Invalidate affected frames when a range in a selection is modified

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Mats Palmgren (inactive)]

The following Range methods doesn't invalidate the selection properly:
SetStart
  SetStartBefore
  SetStartAfter
SetEnd
  SetEndBefore
  SetEndAfter
Collapse
SelectNode
SelectNodeContents
SurroundContents
Detach

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix + tests, v1

Call InvalidateFrameSubtree() for all continuations of the
range common ancestor before/after the modification as needed.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 570516 [details] [diff] [review]
fix + tests, v1


>+static void InvalidateAllFrames(nsINode* aNode)
>+{
>+  NS_PRECONDITION(aNode, "bad arg");
>+
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
QI is a bit slow, and adds extra addref/release.

Could you use non-virtual nsINode::NodeType()
and switch-case.
DOCUMENT_NODE should cause invalidation to root frame, 
ATTRIBUTE_NODE, ENTITY_REFERENCE_NODE, ENTITY_NODE, NOTATION_NODE, DOCUMENT_FRAGMENT_NODE
should not cause any validation, and for the rest casting to nsIContent* should be safe.

> nsRange::SetStart(nsIDOMNode* aParent, PRInt32 aOffset)
> {
>   VALIDATE_ACCESS(aParent);
> 
>   nsCOMPtr<nsINode> parent = do_QueryInterface(aParent);
>+  AutoInvalidateSelection atEndOfBlock(this);
>   return SetStart(parent, aOffset);
> }

Could we just do the whole AutoInvalidateSelection thing in DoSetRange?

Does this affect badly to performance? IIRC we have some selection handling related
perf bugs open when one selects a huge table or so.

r- because I want QI of cycle collected objects reduced.

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix + tests, v2

> Could you use non-virtual nsINode::NodeType() and switch-case.

Fixed.  I think we only need to care about TEXT and ELEMENT (nsIContent)
and DOCUMENT (nsIDocument).  I don't think we can have frames for the
other ones(?)

> Could we just do the whole AutoInvalidateSelection thing in DoSetRange?

That would be overkill.  When removing/adding/changing nodes the frames are
already invalidated by other code.  Same for Selection methods which does
its invalidation in selectFrames().  Some methods also calls DoSetRange
multiple times.  Still, I agree it would be nice to consolidate the
invalidation part in one place.  I think a better way to do that though
is to implement the :selection pseudo-class and let the style system do
the updates.

> Does this affect badly to performance?

The performance impact should be negligible I think, I don't see how we can
fix this bug without some sort of frame invalidation code.

> IIRC we have some selection handling
> related perf bugs open when one selects a huge table or so.

Let me know if you have know the specific bug numbers and I'll test it -
I'll query Bugzilla to see what I can find.

(FYI, I'm in New Zealand time zone the next two weeks)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 574179 [details] [diff] [review]
fix + tests, v2

 
>+  AutoInvalidateSelection atEndOfBlock(this);
Not having AutoInvalidateSelection in DoSetRange looks a bit error prone.
But I can see the performance reasons to not do that.

>+nsRange::GetRegisteredCommonAncestor()
>+{
>+  NS_ASSERTION(IsInSelection(),
>+               "GetRegisteredCommonAncestor only valid for range in selection");
>+  nsINode* ancestor = GetNextRangeCommonAncestor(mStartParent);
>+  while (ancestor) {
>+    RangeHashTable* ranges =
>+      static_cast<RangeHashTable*>(ancestor->GetProperty(nsGkAtoms::range));
>+    if (ranges->GetEntry(this)) {
>+      break;
>+    }
>+    ancestor = GetNextRangeCommonAncestor(ancestor->GetNodeParent());
>+  }
This could be reasonable slow. We have still plenty of spare boolean flags in nsINode, so would it
make sense to add a flag if node may have a range in its RangeHashTable?
Though, actually, perhaps there should be flag to indicate if node has any properties.
Could you file a followup to investigate how often nodes have properties (I mean any property) and how often
GetProperty is used on nodes which don't have any properties.

>+  nsINode* GetRegisteredCommonAncestor();
Please document what this method does.

>+  struct NS_STACK_CLASS AutoInvalidateSelection {
>+    AutoInvalidateSelection(nsRange* aRange) : mRange(aRange) {
{ should be in the next line.


>+#ifdef DEBUG
>+      mWasInSelection = mRange->IsInSelection();
>+#endif
>+      if (!mRange->IsInSelection() || mIsNested)
>+        return;
if (expr) {
  stmt;
}

Comment 5

[Mats Palmgren (inactive)]

> >+nsRange::GetRegisteredCommonAncestor()
> >+{
> >+  NS_ASSERTION(IsInSelection(),
> >+               "GetRegisteredCommonAncestor only valid for range in selection");
> >+  nsINode* ancestor = GetNextRangeCommonAncestor(mStartParent);
> >+  while (ancestor) {
> >+    RangeHashTable* ranges =
> >+      static_cast<RangeHashTable*>(ancestor->GetProperty(nsGkAtoms::range));
> >+    if (ranges->GetEntry(this)) {
> >+      break;
> >+    }
> >+    ancestor = GetNextRangeCommonAncestor(ancestor->GetNodeParent());
> >+  }
> This could be reasonable slow. We have still plenty of spare boolean flags
> in nsINode, so would it
> make sense to add a flag if node may have a range in its RangeHashTable?

That shouldn't be a problem here, GetNextRangeCommonAncestor returns the 
next ancestor with the CommonAncestorForRangeInSelection bit and if non-null
that ancestor MUST have this property with at least one range in the hash table.
It's rare to have more than one such ancestor (overlapping ranges).

> Though, actually, perhaps there should be flag to indicate if node has any
> properties.
> Could you file a followup to investigate how often nodes have properties (I
> mean any property) and how often
> GetProperty is used on nodes which don't have any properties.

Sure, that could be an optimization in general.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 2. remove wallpaper for this bug in an earlier reftest

Comment 7

[Mats Palmgren (inactive)]

With the nits fixed as suggested, and the wallpaper in an earlier reftest removed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91909393c5a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2c62d75dea

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/91909393c5a0
https://hg.mozilla.org/mozilla-central/rev/4b2c62d75dea