Closed Bug 671152 Opened 13 years ago Closed 2 years ago

ASSERTION: null node passed to IsTextNode() + ASSERTION: selection could not be collapsed after undo of deletetext.

Categories

(Core :: DOM: Editor, defect, P5)

Product:
Core
Component:
DOM: Editor
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: MatsPalmgren_bugz, Unassigned)

References

Details

(Keywords: assertion, Whiteboard: [fuzzblocker])

Attachments

(1 file)

testcase.html
788 bytes, text/html
Details
STEPS TO REPRODUCE 1. run mochitest editor/libeditor/html/tests/test_bug537046.html ACTUAL RESULTS ###!!! ASSERTION: selection could not be collapsed after undo of deletetext.: '(NS_SUCCEEDED(result))', file editor/libeditor/base/DeleteTextTxn.cpp, line 125 ###!!! ASSERTION: null node passed to IsTextNode(): 'Not Reached', file editor/libeditor/base/nsEditor.cpp, line 3757 PLATFORMS AND BUILDS TESTED Bug occurs in a local mozilla-central DEBUG build on Linux x86-64
Still happens on mozilla-central.
Blocks: 404077
So the failure is because mAncestorLimiter is set, and the text node is not a descendant of it. I *think* mAncestorLimiter is the editable div, and the text node has no parent at the time the transaction runs. Thus the Collapse() call fails. Ehsan, do you know why this is a problem? Why can't the selection be collapsed into a detached text node? Maybe we don't want it to be in this case, but it should be legal, no? The node and offset are totally valid -- it's a detached Text node with offset 0.
Hmm, how does this happen? Selection is a property of the presshell, right? So it wouldn't make a lot of sense for a detached node to be part of it. Or am I missing something?
Well, we do support selections being in detached nodes: data:text/html,<!DOCTYPE html> <script> var text = document.createTextNode("abc"); getSelection().collapse(text, 0); document.documentElement.textContent = getSelection().anchorNode + " " + getSelection().anchorOffset; </script> Outputs "[object Text] 0" in a nightly for me. This is per spec -- Selections can contain any Range. IE also behaves this way, IIRC, but WebKit does not. Of course, a Selection that's not in a visible node won't itself be visible. So given that the test-case above works, why is this failing? What's setting mAncestorLimiter, and why do we want it?
Huh! So we basically use mAncestorLimit to determine how much of the tree to look at. If selections behave this way, this entire infrastructure will be broken. :( Have you looked into why the Collapse call itself fails though?
(In reply to Ehsan Akhgari [:ehsan] from comment #5) > Huh! So we basically use mAncestorLimit to determine how much of the tree > to look at. If selections behave this way, this entire infrastructure will > be broken. :( I'm not sure what you mean here. Specifically, what's the difference between the selection in the test-case of comment 4 and the test-case in comment 0? Why does the latter have an ancestor limiter, and the former not? > Have you looked into why the Collapse call itself fails though? It checks IsValidSelectionPoint, which verifies that the node is a descendant of aFrameSel->GetAncestorLimiter() if that's set. So it returns NS_ERROR_FAILURE. At least that's what it looks like from code inspection.
(In reply to :Aryeh Gregor from comment #6) > (In reply to Ehsan Akhgari [:ehsan] from comment #5) > > Huh! So we basically use mAncestorLimit to determine how much of the tree > > to look at. If selections behave this way, this entire infrastructure will > > be broken. :( > > I'm not sure what you mean here. Specifically, what's the difference > between the selection in the test-case of comment 4 and the test-case in > comment 0? Why does the latter have an ancestor limiter, and the former not? Because the editor is not involved in the former? What do we expect to happen with the test case in comment 4 though? We seem to be the only browser which allows the selection to be collapsed to a node that is not in the document... > > Have you looked into why the Collapse call itself fails though? > > It checks IsValidSelectionPoint, which verifies that the node is a > descendant of aFrameSel->GetAncestorLimiter() if that's set. So it returns > NS_ERROR_FAILURE. At least that's what it looks like from code inspection. In that case, the caller of Collapse should be able to handle it failing, and should not assert.... :/
(In reply to Ehsan Akhgari [:ehsan] from comment #7) > Because the editor is not involved in the former? Well, yes, but why should that make a difference? Either we support such selections or we don't. Why should we suddenly not support them when the editor is involved, when we otherwise do? > What do we expect to happen with the test case in comment 4 though? We seem > to be the only browser which allows the selection to be collapsed to a node > that is not in the document... Okay, so should we change that? Obviously it's pretty useless. Do we want to limit a window's selection to only be in (inclusive) descendants of that window's document? > In that case, the caller of Collapse should be able to handle it failing, > and should not assert.... :/ This is another case where the assertion is sane but mutation events make it incorrect. I'd really hate to lose sanity checks like this for common cases just because mutation events are broken.
(In reply to :Aryeh Gregor from comment #8) > (In reply to Ehsan Akhgari [:ehsan] from comment #7) > > Because the editor is not involved in the former? > > Well, yes, but why should that make a difference? Either we support such > selections or we don't. Why should we suddenly not support them when the > editor is involved, when we otherwise do? I was talking about what's different in our code, not what we should do. Honestly I think it's pretty crazy to allow selections to be collapsed on stuff which are detached from the document. WebKit and Opera seem to agree. Is this spec'ed somewhere? > > What do we expect to happen with the test case in comment 4 though? We seem > > to be the only browser which allows the selection to be collapsed to a node > > that is not in the document... > > Okay, so should we change that? Obviously it's pretty useless. Do we want > to limit a window's selection to only be in (inclusive) descendants of that > window's document? That would make way more sense to me than what we currently do. I think selection is a presentation property, and I don't even know what it means to put it on things which are by definition out of the presentation. > > In that case, the caller of Collapse should be able to handle it failing, > > and should not assert.... :/ > > This is another case where the assertion is sane but mutation events make it > incorrect. I'd really hate to lose sanity checks like this for common cases > just because mutation events are broken. Let's see what bz things here.
(In reply to Ehsan Akhgari [:ehsan] from comment #9) > I was talking about what's different in our code, not what we should do. > Honestly I think it's pretty crazy to allow selections to be collapsed on > stuff which are detached from the document. WebKit and Opera seem to agree. > Is this spec'ed somewhere? Yes, selection stuff is in the editing spec: http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#selections I went with the Gecko behavior for the spec because it was simplest, at least vis-a-vis the spec. (Because the spec doesn't care about how it's presented!) But if we want to change it, I'm fine with that -- I'll update the spec. IE10 Developer Preview also doesn't allow such selections. > > Okay, so should we change that? Obviously it's pretty useless. Do we want > > to limit a window's selection to only be in (inclusive) descendants of that > > window's document? > > That would make way more sense to me than what we currently do. I think > selection is a presentation property, and I don't even know what it means to > put it on things which are by definition out of the presentation. It should be the same as putting it in display: none. Or "visibility: hidden; position: absolute; left: -1000000px". No selection is visible, but it still works programmatically. Technically, per current spec, if you collapse() the selection into a text node that's the child of a detached element with contenteditable, typing text will insert text as usual into the text node. This doesn't seem very useful, I grant! (Even if computed styles worked.) I'll change the spec and file a bug to change our code. Probably we have to silently ignore the command, not throw, for compat -- only IE throws here, AFAIK, and its implementation is very new and probably not compatible. Meanwhile, we have to fix this bug. I think the right approach is along the lines of what we took in bug 766426 -- detect the case of mutation events and throw an exception (or otherwise fail) rather than asserting. Actually, I think that in this case we should probably refuse to delete the node at all, because we shouldn't consider detached nodes editable, right?
(In reply to :Aryeh Gregor from comment #10) > Meanwhile, we have to fix this bug. I think the right approach is along the > lines of what we took in bug 766426 -- detect the case of mutation events > and throw an exception (or otherwise fail) rather than asserting. Actually, > I think that in this case we should probably refuse to delete the node at > all, because we shouldn't consider detached nodes editable, right? Oh, wait, no -- I think the issue here is that the deletion of some text triggers the removal of the node via mutation events. So the deletion was legit. We just need to refuse to collapse the selection in this case.
(In reply to comment #10) > (In reply to Ehsan Akhgari [:ehsan] from comment #9) > > I was talking about what's different in our code, not what we should do. > > Honestly I think it's pretty crazy to allow selections to be collapsed on > > stuff which are detached from the document. WebKit and Opera seem to agree. > > Is this spec'ed somewhere? > > Yes, selection stuff is in the editing spec: > > http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#selections > > I went with the Gecko behavior for the spec because it was simplest, at least > vis-a-vis the spec. (Because the spec doesn't care about how it's presented!) > But if we want to change it, I'm fine with that -- I'll update the spec. IE10 > Developer Preview also doesn't allow such selections. Yeah, I think we should change the spec here. > > > Okay, so should we change that? Obviously it's pretty useless. Do we want > > > to limit a window's selection to only be in (inclusive) descendants of that > > > window's document? > > > > That would make way more sense to me than what we currently do. I think > > selection is a presentation property, and I don't even know what it means to > > put it on things which are by definition out of the presentation. > > It should be the same as putting it in display: none. Or "visibility: hidden; > position: absolute; left: -1000000px". No selection is visible, but it still > works programmatically. Technically, per current spec, if you collapse() the > selection into a text node that's the child of a detached element with > contenteditable, typing text will insert text as usual into the text node. > This doesn't seem very useful, I grant! (Even if computed styles worked.) Well, I think this is actually harmful. What happens if the element is removed from the document, an editing command gets invoked, and the element then gets added back? The element has been changed by the command is not what the developer or the user would expect, I don't think! > I'll change the spec and file a bug to change our code. Probably we have to > silently ignore the command, not throw, for compat -- only IE throws here, > AFAIK, and its implementation is very new and probably not compatible. Agreed. > Meanwhile, we have to fix this bug. I think the right approach is along the > lines of what we took in bug 766426 -- detect the case of mutation events and > throw an exception (or otherwise fail) rather than asserting. Actually, I > think that in this case we should probably refuse to delete the node at all, > because we shouldn't consider detached nodes editable, right? Yes, I don't think the node should be removed in this case.
(In reply to :Aryeh Gregor from comment #11) > (In reply to :Aryeh Gregor from comment #10) > > Meanwhile, we have to fix this bug. I think the right approach is along the > > lines of what we took in bug 766426 -- detect the case of mutation events > > and throw an exception (or otherwise fail) rather than asserting. Actually, > > I think that in this case we should probably refuse to delete the node at > > all, because we shouldn't consider detached nodes editable, right? > > Oh, wait, no -- I think the issue here is that the deletion of some text > triggers the removal of the node via mutation events. So the deletion was > legit. We just need to refuse to collapse the selection in this case. Oh hrm, yeah you're right!
(In reply to Ehsan Akhgari [:ehsan] from comment #12) > Yeah, I think we should change the spec here. Test case, for reference: data:text/html,<!doctype html> <iframe></iframe> <script> onload = function() { var result = ""; try { var div = document.createElement("div"); getSelection().collapse(div, 0); result += getSelection().anchorNode; } catch(e) { result += e; } try { var text = document.createTextNode("abc"); getSelection().collapse(text, 0); result += " " + getSelection().anchorNode; } catch(e) { result += e; } try { var iframeDoc = document.body.firstChild.contentDocument; getSelection().collapse(iframeDoc.body, 0); result += " " + getSelection().anchorNode; } catch(e) { result += e; } try { document.body.removeChild(document.body.firstChild); getSelection().collapse(iframeDoc.body, 0); result += " " + getSelection().anchorNode; } catch(e) { result += e; } document.body.textContent = result; }; </script> Chrome 21 dev and Opera Next 12.00 alpha both give "null null null null". IE10 Developer Preview throws in the second case (detached text node) but succeeds in all others. (But then when I adjust the test case slightly it throws in the first case too? Ugh.) Firefox 16.0a1 succeeds in all cases. What should we do if a script does getSelection().getRangeAt(0).setStart(detached, 0)? This isn't a problem for Chrome or Opera, because getRangeAt() returns a copy. But we have to handle it somehow. Actually, if we're going to place any restriction on what ranges can be in selections, I think we're going to have to change getRangeAt() to return a copy. Does that make sense to you? I've gone back and forth on it, but the spec currently matches Gecko here. If you think getRangeAt() should return a copy instead of a reference, so scripts can't directly modify selections' ranges, I'll change the spec and write a patch for Gecko. > Well, I think this is actually harmful. What happens if the element is > removed from the document, an editing command gets invoked, and the element > then gets added back? The element has been changed by the command is not > what the developer or the user would expect, I don't think! This is moot, but -- if the element is removed from the document, range mutation rules will mean the selection gets moved to its parent, so it's no longer selected. The only time a detached element can be in the selection per current spec is if script puts it there.
Ryosuke, you might be interested in above discussion. I think we should change the selection spec to match WebKit a bit better -- make getRangeAt() return a copy (so modifying the result doesn't change the selection), and make addRange/collapse/extend/etc. fail silently if you try to place the selection in anything other than a descendant of window.document. Including if you try to place the selection in an iframe. I'm guessing you don't object, since it just makes WebKit more correct! But I wanted to make sure you're okay with it anyway.
It sounds reasonable to me.
(In reply to comment #14) > What should we do if a script does > getSelection().getRangeAt(0).setStart(detached, 0)? This isn't a problem for > Chrome or Opera, because getRangeAt() returns a copy. But we have to handle it > somehow. Actually, if we're going to place any restriction on what ranges can > be in selections, I think we're going to have to change getRangeAt() to return > a copy. Does that make sense to you? I've gone back and forth on it, but the > spec currently matches Gecko here. If you think getRangeAt() should return a > copy instead of a reference, so scripts can't directly modify selections' > ranges, I'll change the spec and write a patch for Gecko. Yeah, returning a copy makes sense. > > Well, I think this is actually harmful. What happens if the element is > > removed from the document, an editing command gets invoked, and the element > > then gets added back? The element has been changed by the command is not > > what the developer or the user would expect, I don't think! > > This is moot, but -- if the element is removed from the document, range > mutation rules will mean the selection gets moved to its parent, so it's no > longer selected. The only time a detached element can be in the selection per > current spec is if script puts it there. OK, then the above should have us covered, I think.
Blocks: 768756
Whiteboard: [fuzzblocker]
Any updates here?
(In reply to Olli Pettay [:smaug] from comment #18) > Any updates here? As far as I know nobody is currently working on this bug.
> Yeah, returning a copy makes sense. Note that Chrome and Opera now counts as one implementation. IE10 returns a live range afaict. So there are two UAs that implements what the spec says, and one that doesn't. Did we contact Webkit/Blink folks and ask if they intend to implement what the spec says?
(In reply to comment #20) > > Yeah, returning a copy makes sense. > > Note that Chrome and Opera now counts as one implementation. > > IE10 returns a live range afaict. So there are two UAs that implements > what the spec says, and one that doesn't. This doesn't really make this easier to decide. :( > Did we contact Webkit/Blink folks and ask if they intend to implement > what the spec says? See comment 16.
The problem of detached and wrong-document selections leads to a wide variety of assertions. My DOM fuzzer currently ignores all assertions in editor, along with a few assertions in other code. I'd really appreciate a fix that prevents this situation, or at least disables execCommand() when selection endpoints aren't descendants of the document.
I don't think anyone is working on Blink or WebKit to return a live Range so you might want to raise the question in public-webapps to see if Microsoft is interested in changing IE's behavior. FWIW, IE didn't have Gecko compatible API until recently so they might not have much backwards compatibility concern as much as we do.

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.

Severity: normal → S4
Priority: -- → P5

The testcase included in comment 14 no longer reproduces, nor can I find any reference to the assertion message in-tree. I'm going to close this for now.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: