700538 - nsHTMLEditor should override IsRootNode

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

[(no longer active)]

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]]

Attachment: Patch

Comment 3

[(no longer active)]

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]]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/?tree=Try&rev=b906b6ba874d

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[(no longer active)]

(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 (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/?tree=Try&rev=1950052411f1

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[(no longer active)]

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]]

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

[(no longer active)]

(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

[(no longer active)]

Comment on attachment 591052 [details] [diff] [review]
Patch

Clearing the review request for now.

Comment 14

[(no longer active)]

Jignesh, do you plan to continue to work on this?

Comment 15

[Jignesh Kakadiya [:jhk]]

I tried it but result coming is not expected. If someone wants to do it go ahead:).

Comment 16

[(no longer active)]

OK, thanks!  Aryeh, can you take a look at this, please?

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

[(no longer active)]

OK, that sounds good!

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 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 (:ayg) (no longer with Mozilla)]

Attachment: 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 (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v1 -- Clean up some nsEditor methods

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 4, v1 -- Add and use new EditorRoot methods in nsEditor

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 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

[(no longer active)]

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

[(no longer active)]

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

[(no longer active)]

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 (:ayg) (no longer with Mozilla)]

(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 (:ayg) (no longer with Mozilla)]

(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

[(no longer active)]

Hmm, OK I think I read the patch wrong.  There's no QI involved here, so please ignore that comment.  Sorry about that!

Comment 30

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

No problem.  Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/18c27249fc7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/edebd9b79fe5
https://hg.mozilla.org/integration/mozilla-inbound/rev/222fc2b8378f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8396c8f870d3

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/18c27249fc7f
https://hg.mozilla.org/mozilla-central/rev/edebd9b79fe5
https://hg.mozilla.org/mozilla-central/rev/222fc2b8378f
https://hg.mozilla.org/mozilla-central/rev/8396c8f870d3

Comment 32

[Mozilla RelEng Bot]

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.