Closed Bug 1031440 Opened 8 years ago Closed 8 years ago

Start moving mozInlineSpellChecker off of nsIDOM* APIs

Categories

(Core :: DOM: Editor, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mrbkap, Assigned: mrbkap)

Details

Attachments

(3 files)

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.
Assignee: nobody → mrbkap
(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.  ;-)
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.
Attachment #8447341 - Flags: review?(ehsan)
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.
Attachment #8447342 - Flags: review?(ehsan)
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.
Attachment #8447346 - Flags: review?(ehsan)
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.  :(
Attachment #8447341 - Flags: review?(ehsan) → review+
Attachment #8447342 - Flags: review?(ehsan) → review+
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&.
Attachment #8447346 - Flags: review?(ehsan) → review+
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 on attachment 8447346 [details] [diff] [review]
Switch to internal ranges and selection interfaces

>   nsCOMPtr<nsIDOMRange> checkRange;
...
>+    nsRange *checkRange = ranges[idx];
Unused variable?
(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.
https://hg.mozilla.org/mozilla-central/rev/d774d2c87e78
https://hg.mozilla.org/mozilla-central/rev/95c7e00d68b4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.