1004522 - Finish cleaning up nsWSRunObject

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=4ef871d1619d

After this patch set, nsWSRunObject.cpp no longer uses nsIDOMFoo internally, only a) in one constructor variant (which I'd like to remove), and b) to call a few outside methods that still take nsIDOMFoo.  All other methods have been updated, and all callers have been updated (crudely) to pass in nsINode and friends.

This is my last patchset for this break.  I'll get the patches checked as time permits, once they're reviewed.  My next break is from August 6 to 26 or thereabouts.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 -- Clean up nsHTMLEditRules::JoinBlocks

kungFuDeathGrip is the correct replacement for all the NS_ENSURE_STATE(mHTMLEditor), correct?

I removed the nsAutoTrackDOMPoint because a) it doesn't work with an Element pointer (it might want to update to a non-Element), and b) it seems to only change text, and we're passing Elements, so it should never change them.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 --- Clean up nsWSRunObject::PrepareToJoinBlocks

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 -- Clean up nsWSRunObject::DeleteWSForward()

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 4 -- Clean up nsWSRunObject::PrepareToDeleteRange

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 5 -- Clean up nsWSRunObject::PrepareToDeleteNode

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 6 -- Clean up nsWSRunObject::PrepareToSplitAcrossBlocks

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 7 -- Clean up nsWSRunObject::InsertText

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 8 -- Clean up nsWSRunObject::Prior/NextVisibleNode

The changes to callers here get a bit messy, but IMO it's better than keeping a second nsIDOMNode* variant of the methods lying around.  I'll get around to fixing up the callers, no worries.

(This isn't the last in the patch set, there are still six to go.  But I have to run now, I'll post the rest later.)

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8415918 [details] [diff] [review]
Part 1 -- Clean up nsHTMLEditRules::JoinBlocks

This currently causes a crash that will take time to debug (pure virtual function call during garbage collection), so don't bother reviewing it just yet.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 9 -- Clean up nsWSRunObject::GetCharBefore/After

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 10 -- Clean up nsWSRunObject::FindRun

Hmm, I should probably have made this return WSFragment*.

If you pass a null node to the nsIDOMNode* version of nsContentUtils::ComparePoints, it returns -1.  If you pass it to the nsINode* version, it crashes.  Just so you know (sigh).

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 11 -- Clean up nsWSRunObject::CheckLeading/TrailingNBSP, ConvertToNBSP

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 12 -- Convert nsWSRunObject.mNodeArray to nsTArray<nsCOMPtr<Text> >

I honestly did not realize until I wrote this patch that the array consisted only of text nodes.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 13 -- Clean up nsWSRunObject::DeleteChars

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 14 -- Clean up nsWSRunObject::GetAsciiWSBounds

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 15 -- Clean up nsWSRunObject::GetWSPointBefore/After

Last patch.

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

New try with some patches updated: https://tbpl.mozilla.org/?tree=Try&rev=b2a40b504888

Comment 18

[(no longer active)]

Hmm, I was going to start reviewing the patches but I noticed a few crashes on the try push.  Can you please take a look

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I'm doing so, but bisecting locally made me pretty confident that they're all in the first patch, so I *think* it should be reasonable to review the others.

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8415918 [details] [diff] [review]
Part 1 -- Clean up nsHTMLEditRules::JoinBlocks

So it *looks* like this introduces a use-after-free somewhere, specifically multiple-release of an Element.  I looked over it a couple of times and can't find anything that could be responsible.  I'll look some more, but if you can find it, please let me know.  Maybe it's obvious to you.  If I can't find it, I might have to rewrite the patch.

Comment 21

[(no longer active)]

Comment on attachment 8415918 [details] [diff] [review]
Part 1 -- Clean up nsHTMLEditRules::JoinBlocks

Review of attachment 8415918 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I don't see anything obviously wrong here, but these types of issues are very difficult to catch with naked eyes.  You can try a tool like valgrind or asan <https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer> to help you diagnose where the free happens.  My guess is that you need to hold a strong reference to an element somewhere...

Comment 22

[(no longer active)]

(In reply to :Aryeh Gregor from comment #11)
> If you pass a null node to the nsIDOMNode* version of
> nsContentUtils::ComparePoints, it returns -1.  If you pass it to the
> nsINode* version, it crashes.  Just so you know (sigh).

That's very annoying!  Ironically if you pass nullptr to both nsINode* arguments, you get 0.  Boris, worth filing a follow-up here?

Comment 23

[(no longer active)]

Comment on attachment 8416445 [details] [diff] [review]
Part 10 -- Clean up nsWSRunObject::FindRun

Review of attachment 8416445 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsWSRunObject.cpp
@@ +1663,4 @@
>    *outRun = nullptr;
> +
> +  for (WSFragment* run = mStartRun; run; run = run->mRight) {
> +    int16_t comp = run->mStartNode ? nsContentUtils::ComparePoints(aNode,

Why not int32_t, which is what ComparePoints returns?

Comment 24

[(no longer active)]

Comment on attachment 8416452 [details] [diff] [review]
Part 12 -- Convert nsWSRunObject.mNodeArray to nsTArray<nsCOMPtr<Text> >

Review of attachment 8416452 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsWSRunObject.h
@@ +382,5 @@
>      nsCOMPtr<nsINode> mLastNBSPNode;   // location of last nbsp in ws run, if any
>      int32_t mLastNBSPOffset;           // ...
>      
> +    // the list of nodes containing ws in this run
> +    nsTArray<nsCOMPtr<mozilla::dom::Text> > mNodeArray;

Nit: just use >> (yes, we can do that these days!)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Followup is probably ok, but in general it seems to me that passing null to ComparePoints is a programming error...  If you don't have a node, you don't have a point, and then what are you comparing to what, and why would any answer make sense?

Comment 26

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #25)
> Followup is probably ok, but in general it seems to me that passing null to
> ComparePoints is a programming error...  If you don't have a node, you don't
> have a point, and then what are you comparing to what, and why would any
> answer make sense?

I agree, but the fact that the nsIDOMPoint* version does accept null nodes makes that very difficult to catch.  I wonder how many existing call sites we have which are affected...

Comment 27

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I just dropped the first patch from the series for now rather than trying to make it work.  So for the checkin, all patches are renumbered with the first dropped.

https://tbpl.mozilla.org/?tree=Try&rev=2a4aaf2cb0b6

This has some failures, which I reproduced locally, tracked down, and fixed.  They were due to two very silly typos ("left" twice instead of "left" then "right"), neither of which was in the patches posted here, so I can't blame Ehsan.  :)

Will push to inbound as soon as the tree opens.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #26)
> I agree, but the fact that the nsIDOMPoint* version does accept null nodes
> makes that very difficult to catch.

Also, the two versions should at least match, because otherwise code will mysteriously change when you switch from nsIDOMNode* to nsINode*, as it did for me.  It took a while for me to figure it out.

Comment 28

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=31c65c736f32

Comment 29

[Carsten Book [:Tomcat]]

(In reply to :Aryeh Gregor from comment #28)
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?changeset=31c65c736f32

had to back this out for testfailures like https://tbpl.mozilla.org/php/getParsedLog.php?id=39476484&tree=Mozilla-Inbound

Comment 30

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Blech, sorry, that was total negligence on my part -- it conflicted with a different patch and I didn't test the merge.

Comment 31

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Green try: https://tbpl.mozilla.org/?tree=Try&rev=a9092a7153fa

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=bd13360e86ae

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f67fe08ad3cd
https://hg.mozilla.org/mozilla-central/rev/a4f276657b24
https://hg.mozilla.org/mozilla-central/rev/4f05139ccffa
https://hg.mozilla.org/mozilla-central/rev/bbc8c2e9d9ae
https://hg.mozilla.org/mozilla-central/rev/a864252d60e0
https://hg.mozilla.org/mozilla-central/rev/d535ec1c6ca1
https://hg.mozilla.org/mozilla-central/rev/05b6e7663f2e
https://hg.mozilla.org/mozilla-central/rev/60fead8277d4
https://hg.mozilla.org/mozilla-central/rev/afc04ccf011c
https://hg.mozilla.org/mozilla-central/rev/1f99115ebad5
https://hg.mozilla.org/mozilla-central/rev/aa8465742352
https://hg.mozilla.org/mozilla-central/rev/95c9ec6977cb
https://hg.mozilla.org/mozilla-central/rev/39ec6a51421e
https://hg.mozilla.org/mozilla-central/rev/bd13360e86ae