Last Comment Bug 671672 - Reduce a bunch of console spam in debug builds caused by the HTML editor
: Reduce a bunch of console spam in debug builds caused by the HTML editor
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-14 14:06 PDT by :Ehsan Akhgari
Modified: 2011-07-19 07:23 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (18.44 KB, patch)
2011-07-14 14:07 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v2) (19.09 KB, patch)
2011-07-15 08:50 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-07-14 14:06:58 PDT
A bunch of stuff that we whine about on the console are actually expected, and dealt with correctly...
Comment 1 :Ehsan Akhgari 2011-07-14 14:07:38 PDT
Created attachment 545999 [details] [diff] [review]
Patch (v1)
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-14 16:58:08 PDT
Comment on attachment 545999 [details] [diff] [review]
Patch (v1)

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

::: content/html/document/src/nsHTMLDocument.cpp
@@ +2586,5 @@
>  
>          rv = range->SelectNode(node);
> +        if (NS_FAILED(rv)) {
> +          return;
> +        }

Why would this fail? At least add a comment.

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +1240,5 @@
>    res = aSelection->GetIsCollapsed(&bCollapsed);
>    NS_ENSURE_SUCCESS(res, res);
> +  if (!bCollapsed) {
> +    return NS_OK;
> +  }

From the comment, it sounds like this is something we're not handling correctly.

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +4288,5 @@
>    }
>    nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
> +  if (!node) {
> +    return PR_FALSE;
> +  }

This should be an assertion, it should never fail. Unless we're passing null to it, but surely that's not supposed to happen?
Comment 3 :Ehsan Akhgari 2011-07-15 08:50:01 PDT
Created attachment 546165 [details] [diff] [review]
Patch (v2)

(In reply to comment #2)
> Comment on attachment 545999 [details] [diff] [review] [review]
> Patch (v1)
> 
> Review of attachment 545999 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/document/src/nsHTMLDocument.cpp
> @@ +2586,5 @@
> >  
> >          rv = range->SelectNode(node);
> > +        if (NS_FAILED(rv)) {
> > +          return;
> > +        }
> 
> Why would this fail? At least add a comment.

Because the node might no longer be in the document.  For example, if we have a contenteditable div, and we set the innerHTML property on a parent of it, the node gets detached from the document, and then this function is called on it to make it non-editable.  We don't need to spell check these types of nodes, so ignoring this failure is safe here.

> ::: editor/libeditor/html/nsHTMLEditRules.cpp
> @@ +1240,5 @@
> >    res = aSelection->GetIsCollapsed(&bCollapsed);
> >    NS_ENSURE_SUCCESS(res, res);
> > +  if (!bCollapsed) {
> > +    return NS_OK;
> > +  }
> 
> From the comment, it sounds like this is something we're not handling
> correctly.

Well, what this comment really means is that we just ignore non-collapsed selections here.  I modified the comment to make it clearer.

> ::: editor/libeditor/html/nsHTMLEditor.cpp
> @@ +4288,5 @@
> >    }
> >    nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
> > +  if (!node) {
> > +    return PR_FALSE;
> > +  }
> 
> This should be an assertion, it should never fail. Unless we're passing null
> to it, but surely that's not supposed to happen?

Yes, it would fail if we pass null to the function.  Specifically, this can happen here: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#5768 in case the start/end node does not have any children.  I think we can allow this function to deal with null input, although it might make a bit more sense to move this change to be the first thing happening in the function.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-15 13:54:07 PDT
Comment on attachment 546165 [details] [diff] [review]
Patch (v2)

Review of attachment 546165 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 5 Joe Drew (not getting mail) 2011-07-16 18:47:27 PDT
This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.
Comment 6 Marco Bonardo [::mak] 2011-07-19 07:23:57 PDT
http://hg.mozilla.org/mozilla-central/rev/bb0926cfe5f8

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