The default bug view has changed. See this FAQ.

Reduce a bunch of console spam in debug builds caused by the HTML editor

RESOLVED FIXED in mozilla8

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla8
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
A bunch of stuff that we whine about on the console are actually expected, and dealt with correctly...
(Assignee)

Comment 1

6 years ago
Created attachment 545999 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #545999 - Flags: review?(roc)
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?
(Assignee)

Comment 3

6 years ago
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.
Attachment #545999 - Attachment is obsolete: true
Attachment #546165 - Flags: review?(roc)
Attachment #545999 - Flags: review?(roc)
Comment on attachment 546165 [details] [diff] [review]
Patch (v2)

Review of attachment 546165 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #546165 - Flags: review?(roc) → review+
This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.
http://hg.mozilla.org/mozilla-central/rev/bb0926cfe5f8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.