Bug 6109 – khtml/editing should use RefPtr instead of manual ref/deref

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

khtml/editing should use RefPtr instead of manual ref/deref

Patch attached.

[Eric Seidel (no email)]

Created attachment 5106 [details]
Proposed patch.

[Eric Seidel (no email)]

Comment on attachment 5106 [details]
Proposed patch.

Unfortunately this one is the biggest of the bunch, and the most complex.  I
can break it up into smaller chunks if that would be better.

A reviewer should look for the same things as with any RefPtr patch:

1.  Did I mark the same instance variables RefPtr<Foo> as I removed
ref()/deref() methods for?
2.  Did I remember to remove any m_foo(0) initializers
3.  Did I mark some variable m_foo as a RefPtr when it really should have been
a weak reference?

I've of course looked for all these already... but those are the main things
that can go wrong in this kind of patch.

[Eric Seidel (no email)]

Created attachment 5110 [details]
changes in kwq necessary as a result of khtml/editing patch

[Darin Adler]

Comment on attachment 5110 [details]
changes in kwq necessary as a result of khtml/editing patch

The forward looking way to to to this is to change the
_documentFragmentWithImpl parameter to a PassRefPtr -- this completely avoid
reference counting churn.

[Darin Adler]

Comment on attachment 5106 [details]
Proposed patch.

Looks, good. I have many suggestions and I noticed at least one bug (that I
hope would be caught by layout tests!).

First the bug:

In the RemoveNodeCommand constructor, there's a bug that will cause a crash.
You set m_parent to m_removeChild->parentNode() before m_removeChild is
initialized. Also, why not initialize m_refChild just as you do m_parent?

Now the rest of the suggestions:

In the case of StyleChange::init, a forward-looking change would be to change
the function to take a PassRefPtr to the style. The "keepAlive" name for the
RefPtr is a tiny bit misleading, since the primary purpose of doing a ref/deref
pair here is to allow the caller to pass in a just-created style without
ref/deref'ing and know that it will not leak.

The comment you added to ApplyStyleCommand::removeInlineStyle is misleading.
That deref is not part of a "calling convention". The call to deref is there to
balance a call to style = style->copy() at the start of the function. If you
want to clean that up, you could add a new local RefPtr variable. And perhaps
this function as well should take a PassRefPtr?

If computedStyle() was changed to return a PassRefPtr, then we could get rid of
the computedStyle local variables entirely in
DeleteSelectionCommand::saveTypingStyle, which would be even better. Same thing
in EditCommand::styleAtPosition and createMarkup and in some
ReplaceSelectionCommand code.

This line of code is entirely unneeded:

+	 RefPtr<CSSMutableStyleDeclarationImpl> oldStyle = m_typingStyle;

That's just a remnant of the old code.

It's extremely dangerous that there's a CSSComputedStyleDeclarationImpl
endingStyle declared here that's a local. We need to create that with new and
put it in a RefPtr instead because if anyone does a ref/deref on it, there will
be an attempt to delete it!

There's no need for:

+    if (m_typingStyle != style)

in EditCommand::assignTypingStyle.

In general, I think the functions like assignTypingStyle and setStartNode
should just go away completely and be replaced by simple assignment, unless
they are part of the public interface.

~InsertIntoTextNodeCommand, ~InsertNodeBeforeCommand, ~JoinTextNodesCommand,
~MergeIdenticalElementsCommand, ~MoveSelectionCommand,
~RemoveCSSPropertyCommand, ~RemoveNodeAttributeCommand, ~RemoveNodeCommand,
~RemoveNodePreservingChildrenCommand, ~ReplacementFragment, ~NodeDesiredStyle,
~ReplaceSelectionCommand, ~SetNodeAttributeCommand, ~SplitElementCommand,
~SplitTextNodeContainingElementCommand, and ~WrapContentsInDummySpanCommand
should be removed entirely; there's no need to declare them and define them.
The assertions at destruction time aren't valuable and these are otherwise
empty.

In selectionStartHasStyle we don't need a local variable for "bool hasStyle".

The line of code that says fragment->setParent(0) should be removed from
createFragmentFromText and createFragmentFromNodeList. Parent is already 0.

Lines like this one:

+    ASSERT(m_element1->nextSibling() == m_element2.get());

should not require a get() call. If they don't work without get(), we should
fix RefPtr by adding the appropriate operator== overloads to make it work.

This line:

+    RefPtr<NodeImpl> holder = insertFragmentForTestRendering().get();

should be instead:

    PassRefPtr<NodeImpl> holder = insertFragmentForTestRendering();

That will avoid reference count churn entirely.

ReplacementFragment::removeNode should probably be changed to just call
node->remove() and take a PassRefPtr parameter.

NodeDesiredStyle(const NodeDesiredStyle &), ~NodeDesiredStyle, and
NodeDesiredStyle::operator= can just be removed; the automatically generated
ones will "just work". In general also the you've been writing operator=
implementations with equality checks that aren't necessary; RefPtr handles that
case.

[Darin Adler]

Comment on attachment 5110 [details]
changes in kwq necessary as a result of khtml/editing patch

Seems best to deploy PassRefPtr here as I suggested.

[Eric Seidel (no email)]

Created attachment 5390 [details]
New combined patch addressing darin's concerns.

[Eric Seidel (no email)]

Comment on attachment 5390 [details]
New combined patch addressing darin's concerns.

Unfortunately this patch has grown rather large.  It passes all the layout
tests and introduces no leaks.	So I think this one should be ready to land.

[Darin Adler]

Comment on attachment 5390 [details]
New combined patch addressing darin's concerns.

~DeleteFromTextNodeCommand, ~InsertIntoTextNodeCommand,
~InsertNodeBeforeCommand, ~JoinTextNodesCommand,
~MergeIdenticalElementsCommand, ~MoveSelectionCommand,
~RebalanceWhitespaceCommand, ~RemoveCSSPropertyCommand,
~RemoveNodeAttributeCommand, ~RemoveNodeCommand,
~RemoveNodePreservingChildrenCommand, ~SetNodeAttributeCommand,
~SplitElementCommand, ~SplitTextNodeCommand,
~SplitTextNodeContainingElementCommand, ~WrapContentsInDummySpanCommand, don't
need to be declared or defined and should be removed from the class
declaration. There's no reason to declare the destructor with an inline empty
body -- just leave it out entirely. The base class already has a virtual
destructor and that's all that's needed to handle destruction properly in cases
like this.

Also, ~ReplacementFragment need not be declared or defined, for similar
reasons.

In the stateStyle function, there's no need for the "state" local variable.

In _createDocumentFragmentWithMarkupString:baseURLString:, it's fine to use
just a RefPtr, it need not be a PassRefPtr to avoid churn if you're not passing
it to another function that takes a PassRefPtr or returning it as a PassRefPtr,
and Maciej opined it was clearer to only use PassRefPtr instead of RefPtr when
it's superior.

The line of code in createFragmentFromText and createFragmentFromNodeList that
says fragment->setParent(0) is no longer needed and should be omitted.

But I think those changes can go in another patch. This looks fine to land as
is, r=me.

I recommend searching for "ref" and "deref" in all the affected source files to
make sure one didn't slip by.

[Eric Seidel (no email)]

(In reply to comment #9)
> (From update of attachment 5390 [details] [edit])
> ~DeleteFromTextNodeCommand, ~InsertIntoTextNodeCommand,
> ~InsertNodeBeforeCommand, ~JoinTextNodesCommand,
> ~MergeIdenticalElementsCommand, ~MoveSelectionCommand,
> ~RebalanceWhitespaceCommand, ~RemoveCSSPropertyCommand,
> ~RemoveNodeAttributeCommand, ~RemoveNodeCommand,
> ~RemoveNodePreservingChildrenCommand, ~SetNodeAttributeCommand,
> ~SplitElementCommand, ~SplitTextNodeCommand,
> ~SplitTextNodeContainingElementCommand, ~WrapContentsInDummySpanCommand, don't
> need to be declared or defined and should be removed from the class
> declaration. There's no reason to declare the destructor with an inline empty
> body -- just leave it out entirely. The base class already has a virtual
> destructor and that's all that's needed to handle destruction properly in cases
> like this.

When I tried removing these, I got compile warnings/errors, because the implicit destructors required 
the actual Type information for whatever the RefPtr's in that class pointed to (for example 
DocumentImpl).  I was uncomfortable pulling in all the additional headers to make this implicit 
construction work, so I just left many of these as is (empty in the .cpp file)

> Also, ~ReplacementFragment need not be declared or defined, for similar
> reasons.
> 
> In the stateStyle function, there's no need for the "state" local variable.

Removed.

> In _createDocumentFragmentWithMarkupString:baseURLString:, it's fine to use
> just a RefPtr, it need not be a PassRefPtr to avoid churn if you're not passing
> it to another function that takes a PassRefPtr or returning it as a PassRefPtr,
> and Maciej opined it was clearer to only use PassRefPtr instead of RefPtr when
> it's superior.

Fixed.

> The line of code in createFragmentFromText and createFragmentFromNodeList that
> says fragment->setParent(0) is no longer needed and should be omitted.

Removed.

> But I think those changes can go in another patch. This looks fine to land as
> is, r=me.
> 
> I recommend searching for "ref" and "deref" in all the affected source files to
> make sure one didn't slip by.

I did.  Only 3 remain, and these are for the QPtrList (clonedNodes) which holds NodeImpl pointers.  
They're manually ref'd before adding and deref'd when removing.   Eventually that should be removed 
as well, but I left it for now.

One more round of tests, then landing.

[Eric Seidel (no email)]

Landed.