Last Comment Bug 757771 - "ASSERTION: invalid end node offset" after select all on input element with maxlength
: "ASSERTION: invalid end node offset" after select all on input element with m...
Status: RESOLVED FIXED
: assertion
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
data:text/html,<input type="text" max...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 01:32 PDT by Simon Montagu :smontagu
Modified: 2012-06-08 04:22 PDT (History)
6 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (1.02 KB, patch)
2012-05-23 02:11 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
Patch part 1, v1 -- Make NS_NewContentIterator and friends infallible (18.09 KB, patch)
2012-06-05 03:21 PDT, :Aryeh Gregor (working until September 2)
bzbarsky: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Patch part 2, v1 -- Make nsEditor::IsEditable(nsIContent) take nsINode instead (4.54 KB, patch)
2012-06-05 03:23 PDT, :Aryeh Gregor (working until September 2)
no flags Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up nsPlaintextEditor::GetTextSelectionOffsets (22.33 KB, patch)
2012-06-05 03:27 PDT, :Aryeh Gregor (working until September 2)
ehsan: review-
Details | Diff | Splinter Review
Patch part 2, v2 -- Now completely rewritten! (23.05 KB, patch)
2012-06-06 08:22 PDT, :Aryeh Gregor (working until September 2)
no flags Details | Diff | Splinter Review
Patch part 2, v1 -- Change various callers to use nsTypedSelection (12.75 KB, patch)
2012-06-07 05:14 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Include nsRange.h from nsTypedSelection.h (1.41 KB, patch)
2012-06-07 05:15 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review
Patch part 4, v3 -- Create new nsContentUtils::GetSelectionInTextControl method (27.88 KB, patch)
2012-06-07 05:21 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review

Description Simon Montagu :smontagu 2012-05-23 01:32:25 PDT
Steps to reproduce:

Load data:text/html,<input type="text" maxlength="4" value="foo" autofocus="autofocus">

Select all

Type any character

Results: ASSERTION: invalid end node offset: 'IsPasswordEditor() || (endNodeOffset == nodeCount-1 || endNodeOffset == 0)', file ...editor/libeditor/text/nsPlaintextEditor.cpp, line 604
Comment 1 Daniel Glazman (:glazou) 2012-05-23 02:11:29 PDT
Created attachment 626371 [details] [diff] [review]
proposed fix

Selecting all sets the selection to (div,0), (div, nodeCount)) where div
is the div container of the editable area in the input. The code in
nsPlaintextEditor::GetTextSelectionOffsets() since it really assumes a non
"All selected" selection... My fix tweaks startNode/endNode and related offsets
if these nodes are equal to the rootNode and if the corresponding real start
and end nodes are text nodes. That does not change at all the behaviour and
ensures endOffset is not -1, hence no more assertion.

Please note this bug does not happen w/o the maxlength attribute because
nsPlaintextEditor::GetTextSelectionOffsets() is called from
nsTextEditRules::TruncateInsertionIfNeeded() only if maxlength is here.

Since nsPlaintextEditor::GetTextSelectionOffsets() is also called in the
case of a password input, please double check that fix with such an element,
I have no extra time myself for that now. Thanks.
Comment 2 Daniel Glazman (:glazou) 2012-05-23 02:12:05 PDT
s/since it really assumes/ is bogus since it really assumes/
Comment 3 :Ehsan Akhgari 2012-05-23 15:02:24 PDT
Thanks Daniel for your patch!

Aryeh, do you have time to finish up the patch and add tests and prepare it for review?
Comment 4 Simon Montagu :smontagu 2012-05-23 22:38:12 PDT
Just FTR, I first saw this on the about:home search box.
Comment 5 Daniel Glazman (:glazou) 2012-05-24 00:46:25 PDT
@ehsan: np

I recommend testing this patch does not change the behaviour
of webapps inserting HTML content inside an <input> element
to fake contenteditable or things like that... I guess a simple
test where the contents of the anonymous <div> inside the <input>
are not simple text nodes but let's say three inline elements like
spans themselves containing non empty text nodes will be enough.
Comment 6 :Ehsan Akhgari 2012-05-24 13:09:42 PDT
AFAIK, web apps are never able to put stuff directory in those anonymous divs, and if they manage to do that, that's a serious bug in our code!
Comment 7 Daniel Glazman (:glazou) 2012-05-24 13:15:37 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #6)

> AFAIK, web apps are never able to put stuff directory in those anonymous
> divs, and if they manage to do that, that's a serious bug in our code!

In fact, they were but they had to ask for Universal Privileges :-)
Still, there were (and probably still are) add-ons relying on that
(through chrome JS) so it's still good to test, IMHO.
Comment 8 :Aryeh Gregor (working until September 2) 2012-05-25 04:04:40 PDT
First I want to fix bug 757371, since it's a regression.  Then I need to spend a lot of my time porting CSS transforms tests, but I can look at this bug when I get sick of that and need a break.
Comment 9 :Aryeh Gregor (working until September 2) 2012-06-05 03:21:38 PDT
Created attachment 630120 [details] [diff] [review]
Patch part 1, v1 -- Make NS_NewContentIterator and friends infallible

This is by advice of Ms2ger.  (It's relevant to this bug because a later editor/ cleanup patch replaces do_CreateInstance with NS_NewContentIterator, which was the only substantial use of nsresult in that particular function, so it allows me to make its return type void.)
Comment 10 :Aryeh Gregor (working until September 2) 2012-06-05 03:23:11 PDT
Created attachment 630121 [details] [diff] [review]
Patch part 2, v1 -- Make nsEditor::IsEditable(nsIContent) take nsINode instead

Why not?  Documents are just not editable.
Comment 11 :Aryeh Gregor (working until September 2) 2012-06-05 03:27:59 PDT
Created attachment 630123 [details] [diff] [review]
Patch part 3, v1 -- Clean up nsPlaintextEditor::GetTextSelectionOffsets

So I meant for this to be a routine cleanup patch before the fix, but it seems like this patch fixes the problem somehow.  Can you figure out how?  I didn't mean to change the logic at all, so I don't know why it would fix anything.

Happily, this function did almost no error-checking before I looked at it, so I was able to get rid of the last bit of error-checking easily enough (see part 1) and then make it return type void.  Down with nsresult!  One thing I'm not sure about -- can GetRoot() ever return null, and if so, how should that be handled?  I just realized that I inadvertently removed a check for that.

Also, this has no tests.  How should I test it?  A crashtest would make sense, but then I can't use synthesizeKey.  But if I use a mochitest, nothing will catch the assertion, right?
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-06-05 03:35:50 PDT
Comment on attachment 630120 [details] [diff] [review]
Patch part 1, v1 -- Make NS_NewContentIterator and friends infallible

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

LGTM!
Comment 13 :Ehsan Akhgari 2012-06-05 07:40:38 PDT
Comment on attachment 630123 [details] [diff] [review]
Patch part 3, v1 -- Clean up nsPlaintextEditor::GetTextSelectionOffsets

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

::: editor/libeditor/text/nsPlaintextEditor.cpp
@@ +547,2 @@
>  
> +  dom::Element* root = GetRoot();

Yeah, it's possible for GetRoot to return null.

@@ +572,2 @@
>        }
> +      totalLength += node->Length();

This is where you're changing the logic.  Now, if IsEditable returns false, this entire block would be skipped, so endOffset will not be assigned to.
Comment 14 :Ehsan Akhgari 2012-06-05 07:41:13 PDT
Comment on attachment 630121 [details] [diff] [review]
Patch part 2, v1 -- Make nsEditor::IsEditable(nsIContent) take nsINode instead

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

I don't really see the point behind this patch.  What's the benefit of doing this?
Comment 15 :Ehsan Akhgari 2012-06-05 07:45:59 PDT
Comment on attachment 630120 [details] [diff] [review]
Patch part 1, v1 -- Make NS_NewContentIterator and friends infallible

FWIW, I'd have very much liked to see content iterators being converted to not use XPCOM, and instead be used as concrete classes.  This way callers like the one in this bug can just create the object on the stack, and callers which really need dynamic allocation can just use operator new directly...
Comment 16 :Ehsan Akhgari 2012-06-05 07:46:52 PDT
(In reply to Aryeh Gregor from comment #11)
> Also, this has no tests.  How should I test it?  A crashtest would make
> sense, but then I can't use synthesizeKey.  But if I use a mochitest,
> nothing will catch the assertion, right?

Just create a mochitest.  One day we'll start tracking assertions in mochitests as well (bug 404077).
Comment 17 Boris Zbarsky [:bz] 2012-06-05 08:59:03 PDT
Comment on attachment 630120 [details] [diff] [review]
Patch part 1, v1 -- Make NS_NewContentIterator and friends infallible

r=me
Comment 18 :Aryeh Gregor (working until September 2) 2012-06-05 12:08:30 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Yeah, it's possible for GetRoot to return null.

When would this happen, out of curiosity?  I'm assuming it's okay if I just return in this case and leave the return type as void, rather than still returning an nsresult, because all callers ignored the return value anyway.  (Which is why the patch compiles!)

> @@ +572,2 @@
> >        }
> > +      totalLength += node->Length();
> 
> This is where you're changing the logic.  Now, if IsEditable returns false,
> this entire block would be skipped, so endOffset will not be assigned to.

Ah!  I was misreading it.  I'll attach a new patch, and then something to actually fix the error.  It's bad that this bug didn't cause a regression on any tests.  I'll see if I can add a test that catches the bug I introduced too.

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> I don't really see the point behind this patch.  What's the benefit of doing
> this?

The benefit is that patch part three calls IsEditable() on the result of iter->GetCurrentNode(), which is an nsINode*, not nsIContent*.  So I'd need to QI to nsIContent in the caller otherwise.  It seems easier to just change IsEditable, unless there's some reason not to.  Note that the other version of IsEditable takes nsIDOMNode, which generally corresponds to nsINode, so this will be a recurring issue as more nsIDOMNode users are converted to nsINode.

(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> FWIW, I'd have very much liked to see content iterators being converted to
> not use XPCOM, and instead be used as concrete classes.  This way callers
> like the one in this bug can just create the object on the stack, and
> callers which really need dynamic allocation can just use operator new
> directly...

Okay, I'll see if I can figure out how to do that instead.

(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> Just create a mochitest.  One day we'll start tracking assertions in
> mochitests as well (bug 404077).

Okay.  One day . . . :)
Comment 19 :Ehsan Akhgari 2012-06-05 13:38:10 PDT
(In reply to Aryeh Gregor from comment #18)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > Yeah, it's possible for GetRoot to return null.
> 
> When would this happen, out of curiosity?

One example would be when you don't have a document element.  ;-)

>  I'm assuming it's okay if I just
> return in this case and leave the return type as void, rather than still
> returning an nsresult, because all callers ignored the return value anyway. 
> (Which is why the patch compiles!)

Yes!

> > @@ +572,2 @@
> > >        }
> > > +      totalLength += node->Length();
> > 
> > This is where you're changing the logic.  Now, if IsEditable returns false,
> > this entire block would be skipped, so endOffset will not be assigned to.
> 
> Ah!  I was misreading it.  I'll attach a new patch, and then something to
> actually fix the error.  It's bad that this bug didn't cause a regression on
> any tests.  I'll see if I can add a test that catches the bug I introduced
> too.

Yeah, our coverage is long ways to be any good...  :/

> (In reply to Ehsan Akhgari [:ehsan] from comment #14)
> > I don't really see the point behind this patch.  What's the benefit of doing
> > this?
> 
> The benefit is that patch part three calls IsEditable() on the result of
> iter->GetCurrentNode(), which is an nsINode*, not nsIContent*.  So I'd need
> to QI to nsIContent in the caller otherwise.  It seems easier to just change
> IsEditable, unless there's some reason not to.  Note that the other version
> of IsEditable takes nsIDOMNode, which generally corresponds to nsINode, so
> this will be a recurring issue as more nsIDOMNode users are converted to
> nsINode.

Hmm, but wouldn't that cause the rest of the callers who already have an nsIContent* to do an unnecessary QI?

> (In reply to Ehsan Akhgari [:ehsan] from comment #15)
> > FWIW, I'd have very much liked to see content iterators being converted to
> > not use XPCOM, and instead be used as concrete classes.  This way callers
> > like the one in this bug can just create the object on the stack, and
> > callers which really need dynamic allocation can just use operator new
> > directly...
> 
> Okay, I'll see if I can figure out how to do that instead.

Doesn't need to be in this bug, FWIW.  :-)

> (In reply to Ehsan Akhgari [:ehsan] from comment #16)
> > Just create a mochitest.  One day we'll start tracking assertions in
> > mochitests as well (bug 404077).
> 
> Okay.  One day . . . :)

Yeah, but please add the test in this bug!  :-)
Comment 20 :Aryeh Gregor (working until September 2) 2012-06-06 06:28:16 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> This is where you're changing the logic.

Plus there was the minor detail of

  PRInt32 focusOffset = aSelection->GetAnchorOffset();

which most likely contributed to incorrect behavior as well, I'd think.  :/

I've noticed that the logic here completely duplicates that of nsTextControlFrame::DOMPointToOffset, and is significantly more complicated.  The latter is used for the DOM properties .selectionStart and .selectionEnd, among other things.  It would be nice to unify this code, although I'm not sure where to put it.

(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> One example would be when you don't have a document element.  ;-)

How could this method be called in that case?  If something grabs a reference to the editor and then the document goes away and then they call something on the editor?  Does all of editor/ really have to handle that case?  If so, is it okay to handle it by "do whatever you like as long as you don't crash"?  (Like in this case, doing NS_ENSURE_TRUE but not handling the error.)

> >  I'm assuming it's okay if I just
> > return in this case and leave the return type as void, rather than still
> > returning an nsresult, because all callers ignored the return value anyway. 
> > (Which is why the patch compiles!)
> 
> Yes!

Except they weren't actually ignoring the error.  So I'll leave it nsresult for the sake of this one null check, I guess, unless you think it's okay to not do any error handling in this case beyond "don't crash".

> Yeah, our coverage is long ways to be any good...  :/

If the code were the same as used by .selectionStart/.selectionEnd, I bet it would already be covered.

> Hmm, but wouldn't that cause the rest of the callers who already have an
> nsIContent* to do an unnecessary QI?

Yes.  I could add nsINode::AsContent instead, like nsINode::AsElement -- how about that?  That would be nicer to use anyway.

> > (In reply to Ehsan Akhgari [:ehsan] from comment #15)
> > > FWIW, I'd have very much liked to see content iterators being converted to
> > > not use XPCOM, and instead be used as concrete classes.  This way callers
> > > like the one in this bug can just create the object on the stack, and
> > > callers which really need dynamic allocation can just use operator new
> > > directly...
> > 
> > Okay, I'll see if I can figure out how to do that instead.
> 
> Doesn't need to be in this bug, FWIW.  :-)

That's good, because I realized after I tried that we'd need an nsContentIterator.h, right?  That's a separate project.

> Yeah, but please add the test in this bug!  :-)

Of course.
Comment 21 :Ehsan Akhgari 2012-06-06 07:49:33 PDT
(In reply to Aryeh Gregor from comment #20)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > This is where you're changing the logic.
> 
> Plus there was the minor detail of
> 
>   PRInt32 focusOffset = aSelection->GetAnchorOffset();
> 
> which most likely contributed to incorrect behavior as well, I'd think.  :/

Ouch!

> I've noticed that the logic here completely duplicates that of
> nsTextControlFrame::DOMPointToOffset, and is significantly more complicated.
> The latter is used for the DOM properties .selectionStart and .selectionEnd,
> among other things.  It would be nice to unify this code, although I'm not
> sure where to put it.

I'm assuming that you're talking about the code at the bottom of nsTextControlFrame::GetSelectionRange, right?  That should work in this case, but only for plaintext editors.  And looking through the call sites of GetTextSelectionOffsets, it seems like it's only used for those types of editors.  So, how about this:

1. Move the GetSelectionRange logic to nsContentUtils.
2. Remove nsPlaintextEditor::GetTextSelectionOffsets (and nsHTMLEditor::GetTextSelectionOffsets for that matter, which is not even implemented!)
3. In the current call sites for GetTextSelectionOffsets, call the new nsContentUtils method.

> (In reply to Ehsan Akhgari [:ehsan] from comment #19)
> > One example would be when you don't have a document element.  ;-)
> 
> How could this method be called in that case?  If something grabs a
> reference to the editor and then the document goes away and then they call
> something on the editor?  Does all of editor/ really have to handle that
> case?  If so, is it okay to handle it by "do whatever you like as long as
> you don't crash"?  (Like in this case, doing NS_ENSURE_TRUE but not handling
> the error.)

Hmm, for HTML editors, the root can be null simply because you can remove document.documentElement from the tree.  But like I said above, this is only used for plaintext editors.  Plaintext editors should always return non-null from GetRoot().

> > >  I'm assuming it's okay if I just
> > > return in this case and leave the return type as void, rather than still
> > > returning an nsresult, because all callers ignored the return value anyway. 
> > > (Which is why the patch compiles!)
> > 
> > Yes!
> 
> Except they weren't actually ignoring the error.  So I'll leave it nsresult
> for the sake of this one null check, I guess, unless you think it's okay to
> not do any error handling in this case beyond "don't crash".

Seems like we can get rid of this method altogether!  :-)

> > Hmm, but wouldn't that cause the rest of the callers who already have an
> > nsIContent* to do an unnecessary QI?
> 
> Yes.  I could add nsINode::AsContent instead, like nsINode::AsElement -- how
> about that?  That would be nicer to use anyway.

Sure, if it's needed here.
Comment 22 :Aryeh Gregor (working until September 2) 2012-06-06 08:22:29 PDT
Created attachment 630576 [details] [diff] [review]
Patch part 2, v2 -- Now completely rewritten!

This replaces the old patch part 3.  It also makes the old patch part 2 unnecessary, since we no longer call IsEditable.  It also makes the code a lot shorter, simpler, and saner.  (Now can we get rid of that trailing <br>?  :) )

Let me know if you want me to make the return type null again and do NS_ENSURE_TRUE(root,) instead of NS_ERROR_NULL_POINTER.

https://tbpl.mozilla.org/?tree=Try&rev=0a846d3d9b0a
Comment 23 :Ehsan Akhgari 2012-06-06 09:50:21 PDT
Comment on attachment 630576 [details] [diff] [review]
Patch part 2, v2 -- Now completely rewritten!

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

I thought that you're going to do what I suggested in comment 21...
Comment 24 :Aryeh Gregor (working until September 2) 2012-06-06 10:30:32 PDT
Comment on attachment 630576 [details] [diff] [review]
Patch part 2, v2 -- Now completely rewritten!

I was -- you just posted a comment while I was finishing up my patch, so I didn't see it.  New version to come.
Comment 25 :Aryeh Gregor (working until September 2) 2012-06-07 05:14:16 PDT
Created attachment 630928 [details] [diff] [review]
Patch part 2, v1 -- Change various callers to use nsTypedSelection

No effect by itself (although I tested that it compiles, and it's a slight LOC decrease).  It's needed by the last patch in the series.
Comment 26 :Aryeh Gregor (working until September 2) 2012-06-07 05:15:53 PDT
Created attachment 630929 [details] [diff] [review]
Patch part 3, v1 -- Include nsRange.h from nsTypedSelection.h

Including nsTypedSelection.h doesn't actually work without nsRange.h, because it declares an nsRefPtr<nsRange>, which seems to need the full class definition (not sure why).  Tell me if you're not the right person to review this.
Comment 27 :Aryeh Gregor (working until September 2) 2012-06-07 05:21:39 PDT
Created attachment 630930 [details] [diff] [review]
Patch part 4, v3 -- Create new nsContentUtils::GetSelectionInTextControl method

This unifies the logic of nsPlaintextEditor::GetTextSelectionOffsets and nsTextControlFrame::GetSelectionRange, as requested.  None of the assertions seem to get hit in editor tests.  GetTextSelectionOffsets used PRUint32 for its arguments, and GetSelectionRange used PRInt32; I went with PRInt32 mostly because it requires fewer changes (since Range offsets are signed).

Tell me if you're not the right person to review the nsTextControlFrame changes.

Try: https://tbpl.mozilla.org/?tree=Try&rev=eca74186387d
Comment 28 :Ehsan Akhgari 2012-06-07 07:16:31 PDT
Comment on attachment 630929 [details] [diff] [review]
Patch part 3, v1 -- Include nsRange.h from nsTypedSelection.h

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

This is weird...

::: layout/generic/nsTypedSelection.h
@@ +17,5 @@
>  struct CachedOffsetForFrame;
>  class nsAutoScrollTimer;
>  class nsIContentIterator;
>  class nsIFrame;
>  class nsRange;

Please remove this.
Comment 29 :Ehsan Akhgari 2012-06-07 07:22:36 PDT
Comment on attachment 630930 [details] [diff] [review]
Patch part 4, v3 -- Create new nsContentUtils::GetSelectionInTextControl method

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

::: layout/forms/nsTextControlFrame.cpp
@@ +1066,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(frameSel, NS_ERROR_FAILURE);
> +  nsRefPtr<nsTypedSelection> typedSel =
> +    frameSel->GetSelection(nsISelectionController::SELECTION_NORMAL);
> +  NS_ENSURE_TRUE(typedSel, NS_ERROR_FAILURE);

Is there no better way to get an nsTypedSelection from a selection controller? :(
Comment 30 :Aryeh Gregor (working until September 2) 2012-06-07 08:00:28 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #28)
> Comment on attachment 630929 [details] [diff] [review]
> Patch part 3, v1 -- Include nsRange.h from nsTypedSelection.h
> 
> Review of attachment 630929 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is weird...

What's weird?  That it ever worked?

(In reply to Ehsan Akhgari [:ehsan] from comment #29)
> Is there no better way to get an nsTypedSelection from a selection
> controller? :(

Not right now.  We couldn't even do that until bug 693933.  I'd really like a nice, infallible way to turn an nsISelection into an nsTypedSelection!
Comment 32 :Ehsan Akhgari 2012-06-07 08:09:40 PDT
(In reply to Aryeh Gregor from comment #30)
> (In reply to Ehsan Akhgari [:ehsan] from comment #28)
> > Comment on attachment 630929 [details] [diff] [review]
> > Patch part 3, v1 -- Include nsRange.h from nsTypedSelection.h
> > 
> > Review of attachment 630929 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is weird...
> 
> What's weird?  That it ever worked?

No, that nsRefPtr needs to see the class declaration...

> (In reply to Ehsan Akhgari [:ehsan] from comment #29)
> > Is there no better way to get an nsTypedSelection from a selection
> > controller? :(
> 
> Not right now.  We couldn't even do that until bug 693933.  I'd really like
> a nice, infallible way to turn an nsISelection into an nsTypedSelection!

OK, well at least this won't be the ugliest code in Gecko, I guess :)
Comment 33 Boris Zbarsky [:bz] 2012-06-07 08:17:10 PDT
nsRefPtr only needs to see the class decl in its assignment operator, one-arg constructor, and destructor.

So what's really happening, presumably, is that you have a class with an inline (possibly implicit) destructor and an nsRefPtr member.  When that happens, the class destructor, which calls the nsRefPtr destructor, needs to see the class declaration.
Comment 34 :Ehsan Akhgari 2012-06-07 08:31:04 PDT
(In reply to Boris Zbarsky (:bz) from comment #33)
> nsRefPtr only needs to see the class decl in its assignment operator,
> one-arg constructor, and destructor.
> 
> So what's really happening, presumably, is that you have a class with an
> inline (possibly implicit) destructor and an nsRefPtr member.  When that
> happens, the class destructor, which calls the nsRefPtr destructor, needs to
> see the class declaration.

Yeah that's precisely what's happening in that header...  And I assume it's because of the calls to AddRef, right?

Stupid stupid C++ <sigh>

Note You need to log in before you can comment on or make changes to this bug.