Last Comment Bug 700538 - nsHTMLEditor should override IsRootNode
: nsHTMLEditor should override IsRootNode
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 9 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 635618 805668
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-07 18:56 PST by Boris Zbarsky [:bz]
Modified: 2012-10-27 11:44 PDT (History)
7 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (17.30 KB, patch)
2012-01-21 02:59 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (18.11 KB, patch)
2012-01-24 03:31 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch part 1, v1 -- Name some nsEditor methods more consistently (7.67 KB, patch)
2012-05-01 04:11 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Make methods that return editing hosts return dom::Element* instead of nsIContent* (5.59 KB, patch)
2012-05-01 04:13 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up some nsEditor methods (7.51 KB, patch)
2012-05-01 04:13 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 4, v1 -- Add and use new EditorRoot methods in nsEditor (20.26 KB, patch)
2012-05-01 04:14 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 5, v1 -- Remove now-redundant nsHTMLEditor::IsNodeInActiveEditor (15.27 KB, patch)
2012-05-01 04:15 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2011-11-07 18:56:25 PST
And in particular, it should make use of GetActiveEditingHost in there.  Then we can remove most of the 5th patch from bug 635618.
Comment 1 :Ehsan Akhgari 2012-01-20 08:39:48 PST
For any document which is in design mode, or has one or more contenteditable elements, there is one nsHTMLEditor object created.  Therefore, in the contenteditable case, an nsHTMLEditor might be responsible for managing multiple editable regions in the document.  nsHTMLEditor has a notion of "active editing host", which corresponds to the editable region of the document which has the focus and is being edited.  nsHTMLEditor::GetActiveEditingHost() returns the root node of the active editing host region.

nsEditor::IsRootNode is not aware of all this, therefore its implementation for nsHTMLEditor is incorrect.  This causes us to have to check for GetActiveEditingHost all around the code base, for example in this patch: <https://hg.mozilla.org/mozilla-central/rev/68ed3842525d>.  We should override both versions of nsEditor::IsRootNode in nsHTMLEditor, and the new implementation needs compare the node passed in to the result of GetActiveEditingHost().  Once we do that, we can remove most of the checks in <https://hg.mozilla.org/mozilla-central/rev/68ed3842525d> (and maybe other similar checks), because IsRootNode would be doing the check internally.
Comment 2 Jignesh Kakadiya [:jhk] 2012-01-21 02:59:51 PST
Created attachment 590453 [details] [diff] [review]
Patch
Comment 3 :Ehsan Akhgari 2012-01-23 11:42:02 PST
Comment on attachment 590453 [details] [diff] [review]
Patch

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

Thanks a lot for your patch, Jignesh!

The patch looks good, except for the comment below.  Have you run tests with this patch?  It should probably be a good choice if you submit it to the try server, and see if you get any test failures.  Do you have try server access?

::: editor/libeditor/html/nsHTMLEditor.h
@@ +408,2 @@
>  
> +  bool IsRootNode(nsINode *inNode); 

You need to make this method in nsEditor virtual.  Please also add the virtual keyword to the nsHTMLEditor versions of these methods.
Comment 4 Jignesh Kakadiya [:jhk] 2012-01-24 03:31:37 PST
Created attachment 591052 [details] [diff] [review]
Patch

Ehsan,
Our campus firewall blocks ssh push.So I will not be able to push into try for few more days.Can you do it for me? 

Thank you!
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-01-24 03:44:44 PST
https://tbpl.mozilla.org/?tree=Try&rev=b906b6ba874d
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-01-24 07:09:35 PST
Looks like you're hitting

###!!! ASSERTION: We started with a proper descendant of root, and should stop if we ever hit the root, so we better have a descendant of root now!: 'IsDescendantOfBody(parent)', file /builds/slave/try-lnx64-dbg/build/editor/libeditor/base/nsEditor.cpp, line 3309
Comment 7 :Ehsan Akhgari 2012-01-27 09:27:49 PST
(In reply to Ms2ger from comment #6)
> Looks like you're hitting
> 
> ###!!! ASSERTION: We started with a proper descendant of root, and should
> stop if we ever hit the root, so we better have a descendant of root now!:
> 'IsDescendantOfBody(parent)', file
> /builds/slave/try-lnx64-dbg/build/editor/libeditor/base/nsEditor.cpp, line
> 3309

Hmm, the results of the try server job don't seem to be available for some reason.  Do you know which test was triggering this assertion?
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-01-27 13:16:15 PST
https://tbpl.mozilla.org/?tree=Try&rev=1950052411f1
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-01-28 06:53:42 PST
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/editor/libeditor/html/crashtests/582138-1.xhtml | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/editor/libeditor/html/crashtests/682650-1.html | assertion count 11 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/editor/libeditor/base/crashtests/636074-1.html | assertion count 13 is more than expected 6 assertions
Comment 10 :Ehsan Akhgari 2012-01-29 16:01:18 PST
Jignesh, please see comment 9 for the test failures introduced by your patch.

In order to debug these, first run this command to make sure that you can reproduce the test failures locally:

make -C /path/to/objdir crashtest TEST_PATH=editor/crashtests.list

Once you get the same failures locally, try debugging them.  In order to debug them, you can run your patched version of Firefox under a debugger, set a conditional breakpoint on NS_DebugBreak_P with aSeverity==1 as the breakpoint condition.  This breakpoint will only trigger in case there is an assertion.  Once you've done that, open the first HTML file in Firefox and try to investigate why the assertion is happening.

Please let me know if you need help.
Comment 11 Jignesh Kakadiya [:jhk] 2012-01-30 05:01:50 PST
I tried to find out. There is completely different set of tests failed locally.

1 )REFTEST TEST-UNEXPECTED-FAIL | file:///home/jiggy/moxilla/asrc/src/editor/libeditor/html/crashtests/382778-1.html | assertion count 2 is more than expected 0 assertions
2 )REFTEST TEST-UNEXPECTED-FAIL | file:///home/jiggy/moxilla/asrc/src/editor/libeditor/html/crashtests/456727-2.html | assertion count 1 is more than expected 0 assertions

4 )REFTEST TEST-UNEXPECTED-FAIL | file:///home/jiggy/moxilla/asrc/src/editor/libeditor/base/crashtests/382527-1.html | assertion count 2 is more than expected 0 assertions


#1 and #4 failed at
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#3578 and 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4160

#2 failed at http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccTreeWalker.cpp#72 

Let me know if it does help.
Comment 12 :Ehsan Akhgari 2012-01-30 12:47:12 PST
(In reply to Jignesh kakadiya from comment #11)
> I tried to find out. There is completely different set of tests failed
> locally.
> 
> 1 )REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/jiggy/moxilla/asrc/src/editor/libeditor/html/crashtests/382778-
> 1.html | assertion count 2 is more than expected 0 assertions
> 2 )REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/jiggy/moxilla/asrc/src/editor/libeditor/html/crashtests/456727-
> 2.html | assertion count 1 is more than expected 0 assertions
>
> 4 )REFTEST TEST-UNEXPECTED-FAIL |
> file:///home/jiggy/moxilla/asrc/src/editor/libeditor/base/crashtests/382527-
> 1.html | assertion count 2 is more than expected 0 assertions
> 
> 
> #1 and #4 failed at
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#3578 and 
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4160

Hmm, these two assertions are really weird.  Do you get them even without your patch?

> #2 failed at
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/
> nsAccTreeWalker.cpp#72 

What is the call stack leading to this assertion?

You should really get assertions similar to the ones in this log: https://tbpl.mozilla.org/php/getParsedLog.php?id=8892094&tree=Try&full=1  Is the crashtest window focused when you ran the tests locally?
Comment 13 :Ehsan Akhgari 2012-01-31 09:09:35 PST
Comment on attachment 591052 [details] [diff] [review]
Patch

Clearing the review request for now.
Comment 14 :Ehsan Akhgari 2012-04-24 18:28:24 PDT
Jignesh, do you plan to continue to work on this?
Comment 15 Jignesh Kakadiya [:jhk] 2012-04-24 22:21:17 PDT
I tried it but result coming is not expected. If someone wants to do it go ahead:).
Comment 16 :Ehsan Akhgari 2012-04-25 20:06:13 PDT
OK, thanks!  Aryeh, can you take a look at this, please?
Comment 17 :Aryeh Gregor (away until August 15) 2012-04-30 00:43:24 PDT
Okay, so I looked at this and I don't think we actually want to do it.  There's a bunch of code that assumes IsRootNode() deals with the same root as IsDescendantOfBody() and GetRoot() and various other things, all of which refer to the document root.  I think if we're going to do this, we want a new set of methods, like IsEditorRootNode() and IsDescendantOfEditorRoot() and things like that, which are the same as the existing ones in nsEditor but are overridden in nsHTMLEditor.  I'll write something to this effect and see how it goes.
Comment 18 :Ehsan Akhgari 2012-04-30 11:23:37 PDT
OK, that sounds good!
Comment 19 :Aryeh Gregor (away until August 15) 2012-05-01 04:11:02 PDT
Created attachment 619889 [details] [diff] [review]
Patch part 1, v1 -- Name some nsEditor methods more consistently

We had IsDescendantOfBody, IsRootNode, and GetRoot.  I renamed the first two to IsDescendantOfRoot and IsRoot.  A later patch will add EditorRoot variants.
Comment 20 :Aryeh Gregor (away until August 15) 2012-05-01 04:13:15 PDT
Created attachment 619890 [details] [diff] [review]
Patch part 2, v1 -- Make methods that return editing hosts return dom::Element* instead of nsIContent*

This is so that the eventual GetEditorRoot can return a dom::Element like GetRoot.  I'm not sure if the static_cast in GetEditingHost is the right way to do things.  The IDL changes were cargo-culted from dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl.  I didn't rev the UUID because it should be compatible, since we're still returning an nsIContent*, right?  (I'm still not sure when I need to do that, exactly.)
Comment 21 :Aryeh Gregor (away until August 15) 2012-05-01 04:13:41 PDT
Created attachment 619892 [details] [diff] [review]
Patch part 3, v1 -- Clean up some nsEditor methods
Comment 22 :Aryeh Gregor (away until August 15) 2012-05-01 04:14:52 PDT
Created attachment 619894 [details] [diff] [review]
Patch part 4, v1 -- Add and use new EditorRoot methods in nsEditor
Comment 23 :Aryeh Gregor (away until August 15) 2012-05-01 04:15:59 PDT
Created attachment 619895 [details] [diff] [review]
Patch part 5, v1 -- Remove now-redundant nsHTMLEditor::IsNodeInActiveEditor

I realized after the fact that this is the same as the new nsEditor::IsDescendantOfEditorRoot, which I prefer due to more consistent naming.
Comment 24 :Ehsan Akhgari 2012-05-01 11:13:06 PDT
Comment on attachment 619890 [details] [diff] [review]
Patch part 2, v1 -- Make methods that return editing hosts return dom::Element* instead of nsIContent*

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

FWIW, not revving the IID is fine here since Element's vtable just adds to nsIContent's, so that pointer will be binary compatible with the current nsIContent pointer.

::: content/base/src/nsGenericElement.cpp
@@ +1572,3 @@
>      content = parent;
>    }
> +  return static_cast<dom::Element*>(content);

Please use AsElement().
Comment 25 :Ehsan Akhgari 2012-05-02 08:13:58 PDT
Comment on attachment 619894 [details] [diff] [review]
Patch part 4, v1 -- Add and use new EditorRoot methods in nsEditor

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

::: editor/libeditor/base/nsEditor.cpp
@@ +3563,5 @@
> +bool
> +nsEditor::IsEditorRoot(nsINode* aNode)
> +{
> +  NS_ENSURE_TRUE(aNode, false);
> +  nsCOMPtr<nsINode> rootNode = GetEditorRoot();

This QI here is unneeded, please get rid of it.

@@ +3588,5 @@
> +bool
> +nsEditor::IsDescendantOfEditorRoot(nsINode* aNode)
> +{
> +  NS_ENSURE_TRUE(aNode, false);
> +  nsCOMPtr<nsIContent> root = GetEditorRoot();

Same with this one.
Comment 26 :Ehsan Akhgari 2012-05-02 08:16:39 PDT
Comment on attachment 619895 [details] [diff] [review]
Patch part 5, v1 -- Remove now-redundant nsHTMLEditor::IsNodeInActiveEditor

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

Nice!
Comment 27 :Aryeh Gregor (away until August 15) 2012-05-02 23:53:33 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #25)
> This QI here is unneeded, please get rid of it.

What do you mean by QI?  The nsCOMPtr?  I'm not clear on exactly when I should use those.  Do you want just

bool
nsEditor::IsEditorRoot(nsINode* aNode)
{
  NS_ENSURE_TRUE(aNode, false);
  return aNode == GetEditorRoot();
}

instead, and likewise for IsDescendantOfEditorRoot?  (And if so, should I update the IsRoot and IsDescendantOfRoot methods I copied from?)

If that's not what you mean, what do you mean?  I don't know what calls QueryInterface under the covers.
Comment 28 :Aryeh Gregor (away until August 15) 2012-05-06 02:58:11 PDT
(Note: this massively conflicted with bug 750064.  For the checkin, I'm dropping patch #3 because bug 750064 cleaned up the same methods, and the rest I merged.)
Comment 29 :Ehsan Akhgari 2012-05-07 10:48:25 PDT
Hmm, OK I think I read the patch wrong.  There's no QI involved here, so please ignore that comment.  Sorry about that!
Comment 32 Mozilla RelEng Bot 2012-05-11 15:03:46 PDT
Autoland Patchset:
	Patches: 619889, 619890, 619892, 619894, 619895
	Branch: mozilla-central => try
Patch 619889 could not be applied to mozilla-central.
patching file editor/libeditor/base/nsEditor.cpp
Hunk #1 FAILED at 3229
Hunk #2 FAILED at 3267
Hunk #3 FAILED at 3288
Hunk #4 FAILED at 3320
Hunk #5 FAILED at 3363
Hunk #6 FAILED at 3381
Hunk #7 FAILED at 3558
7 out of 7 hunks FAILED -- saving rejects to file editor/libeditor/base/nsEditor.cpp.rej
patching file editor/libeditor/base/nsEditor.h
Hunk #1 FAILED at 533
1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/base/nsEditor.h.rej
patching file editor/libeditor/html/nsHTMLEditRules.cpp
Hunk #1 FAILED at 8395
1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/html/nsHTMLEditRules.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

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