1031440 - Start moving mozInlineSpellChecker off of nsIDOM* APIs

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Reading through this code the other day, I noticed that we're still using nsIDOM* APIs in a bunch of places that we don't have to. I've left some in (I only really cared about the main spellchecking loop), so there are still a few extraneous uses left. I also had to leave all of the places that touch the editor core alone because editor.

Comment 1

[(no longer active)]

(In reply to Blake Kaplan (:mrbkap) from comment #0)
> I also had to leave all of the places that touch
> the editor core alone because editor.

Good reason.  ;-)

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Un nsIDOMify SkipSpellcheckForNode

SkipSpellcheckForNode, despite the name, returns true if we *should* spell check the node. I fixed that.

This also makes it use modern APIs and it looks much cleaner now.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix an XPCOM error

It's illegal to call methods on XPCOM objects whose refcounts are 0 and it's worse to do so with runnables (even though this is main-thread-only) because it introduces possible races.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Switch to internal ranges and selection interfaces

Return values are *so* much nicer than out params, especially when they let us avoid a bunch of refcounting. This also removes a few now-redundant local variables, which is cleaner.

Comment 5

[(no longer active)]

Comment on attachment 8447341 [details] [diff] [review]
Un nsIDOMify SkipSpellcheckForNode

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

Nice!

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +49,2 @@
>  #include "nsRange.h"
> +#include "Selection.h"

Please use mozilla/dom/Selection.h.  Apparently we export that header in both places.  :(

Comment 6

[(no longer active)]

Comment on attachment 8447346 [details] [diff] [review]
Switch to internal ranges and selection interfaces

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

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +1368,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    bool doneChecking;
> +  for (int32_t idx = 0; idx < count; idx++) {
> +    nsRange *checkRange = ranges[idx];

I think |ranges[idx]| here returns a temporary nsRefPtr here which gets destroyed, so technically |checkRange| after this line could be UAF.  Although that of course doesn't happen because the array will still hold the object alive, I think I'd be more comfortable if you made checkRange a nsRefPtr<nsRange>&, or better yet, an auto&.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=c47730dfb13f with that and a missing null check that I noticed. I'll check in once try is green.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d774d2c87e78

Comment 9

[[e-mail]]

Comment on attachment 8447346 [details] [diff] [review]
Switch to internal ranges and selection interfaces

>   nsCOMPtr<nsIDOMRange> checkRange;
...
>+    nsRange *checkRange = ranges[idx];
Unused variable?

Comment 10

[[e-mail]]

(In reply to Ehsan Akhgari from comment #6)
> > +    nsRange *checkRange = ranges[idx];
> 
> I think |ranges[idx]| here returns a temporary nsRefPtr here

No, operator[] returns elem_type& (otherwise you wouldn't be able to change it!) but I don't see why you don't just write status.mRange = ranges[idx]; in the first place.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95c7e00d68b4

Done and done. Thanks for pointing that out.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d774d2c87e78
https://hg.mozilla.org/mozilla-central/rev/95c7e00d68b4