Closed Bug 1004522 Opened 8 years ago Closed 8 years ago

Finish cleaning up nsWSRunObject

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(15 files)

25.41 KB, patch
Details | Diff | Splinter Review
7.01 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
8.06 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
6.67 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
5.23 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
7.54 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
15.65 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
36.15 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
20.02 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
9.33 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
13.43 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
12.02 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
25.76 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
8.16 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
9.58 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8415918 - Flags: review?(ehsan)
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.)
Attachment #8415930 - Flags: review?(ehsan)
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.
Attachment #8415918 - Attachment is obsolete: true
Attachment #8415918 - Flags: review?(ehsan)
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).
Attachment #8416445 - Flags: review?(ehsan)
I honestly did not realize until I wrote this patch that the array consisted only of text nodes.
Attachment #8416452 - Flags: review?(ehsan)
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
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 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.
Attachment #8415918 - Attachment is obsolete: false
Attachment #8415918 - Flags: feedback?(ehsan)
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...
Attachment #8415918 - Flags: feedback?(ehsan)
Attachment #8415919 - Flags: review?(ehsan) → review+
Attachment #8415923 - Flags: review?(ehsan) → review+
Attachment #8415924 - Flags: review?(ehsan) → review+
Attachment #8415925 - Flags: review?(ehsan) → review+
Attachment #8415926 - Flags: review?(ehsan) → review+
Attachment #8415928 - Flags: review?(ehsan) → review+
Attachment #8415930 - Flags: review?(ehsan) → review+
Attachment #8416444 - Flags: review?(ehsan) → review+
(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?
Flags: needinfo?(bzbarsky)
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?
Attachment #8416445 - Flags: review?(ehsan) → review+
Attachment #8416447 - Flags: review?(ehsan) → review+
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!)
Attachment #8416452 - Flags: review?(ehsan) → review+
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?
Flags: needinfo?(bzbarsky)
(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...
Attachment #8416455 - Flags: review?(ehsan) → review+
Attachment #8416457 - Flags: review?(ehsan) → review+
Attachment #8416456 - Flags: review?(ehsan) → review+
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.
(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
Blech, sorry, that was total negligence on my part -- it conflicted with a different patch and I didn't test the merge.
You need to log in before you can comment on or make changes to this bug.