62796 - Crash, bad exceptions following Range.detach()

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[David Flanagan]

After the Range.detach() method is called, calls to any further methods
should throw a DOMException.  The attached test case shows cases where no
exception is thrown, where the wrong type of exception is thrown, and where
mozilla nightly 2000120521 crashes.

Comment 1

[David Flanagan]

Attachment: testcase

Comment 2

[Johnny Stenback (:jst)]

Reassigning to the owner of the Range code.

Comment 3

[anthonyd]

setting milestone, and priority.
anthonyd

Comment 4

[anthonyd]

fix in hand, will attach for r= and sr= from kin and jst.

anthonyd

Comment 5

[anthonyd]

Attachment: patch for more range fixes

Comment 6

[anthonyd]

Attachment: patch with kin's suggestions

Comment 7

[kinmoz]

sr=[e-mail]

Comment 8

[anthonyd]

jst, i need an r= from you.

anthonyd

Comment 9

[Johnny Stenback (:jst)]

Patch looks good to me, r=jst

Comment 10

[anthonyd]

fix checked in.
anthonyd

Comment 11

[cls]

This fix has caused textboxes to stop working throughout the application on BeOS
and linux.  Reopening and will be backing out unless a trivial fix is found
soon.  (Anyone else up at this hour?)
  

Comment 12

[cls]

The textbox problem showed up on my win32 build as well.  The changes have been
backed out.

Comment 13

[anthonyd]

Attachment: new patch to handle input boxes problem

Comment 14

[anthonyd]

Ok, I have a new patch up, that basically does not check for the range being 
detached in setStart() and setEnd().  It doesnt make sense to test for that 
anyway, other wise, how would you be able to reset a range?  And if you cant 
reset a range, what would be the point in detach() in the first place?

anthonyd
also I need r= and sr= again

Comment 15

[Blake Ross]

r=blake

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm... this doesn't seem right.  The point of Range.detach() in the DOM spec
seems to be to tell the implementation that one is done with a range.  (I'm not
sure why it would have that rather than allow the implementation's memory
management to deal with disposing of the range in the normal way.)  Why are
ranges being reused after a call to detach()?

Perhaps, for our own internal use, we need something that's like detach() but
doesn't have the same effect of causing the exceptions?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There's only one place in the entire codebase we call Detach on an nsRange, and
that's in editor/base/nsHTMLEditRules.cpp, line 254 and 255 (in
nsHTMLEditRules::BeforeEdit).  So why don't we just fix the caller and then
check in the original patch?

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems to me, though, that there's a bigger problem here.  mIsPositioned is
initialized to false, and the only way to make it true is by calling DoSetRange,
which is called by SetStart and SetEnd.  This means you're not only throwing the
exceptions after detach has been called - you're also throwing them before the
range has been initialized.  Or did I miss something?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just posted a (somewhat) related question to www-dom:
http://lists.w3.org/Archives/Public/www-dom/2001JanMar/0013.html

Comment 20

[anthonyd]

ok, so it looks like detach is just supposed to be used as a release method.  
which also means that it is a useless method.  the original patch i wrote also 
broke input fields (text areas) because it tries to call setStart and setEnd on 
a un positioned range which i was using to determine if the range is detached.  
this will need to be worked on some more.

anthonyd

Comment 21

[anthonyd]

Attachment: new patch to meet dom specs

Comment 22

[anthonyd]

ok, I've got a new patch for this, which meets the dom spec, and does NOT break 
input fields.  need sr= and r= from everyone again.

anthonyd

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You might also want to fix the code in nsHTMLEditRules that I mentioned above.

Comment 24

[Johnny Stenback (:jst)]

I didn't catch this when I reviewed the earlier patch but I think the changes to
nsRange::ToString() are incorrect, calling ToString() on a detached range should
IMO not throw an exception, it should simply return an empty string, comments?

Comment 25

[anthonyd]

the spec says throw an exception, so i made it throw an exception.
from the spec:
toString 
Returns the contents of a Range as a string. This string contains only the data 
characters, not any markup. 
Return Value 
 DOMString The contents of the Range.
Exceptions 
 DOMException INVALID_STATE_ERR: Raised if detach() has already been invoked on 
this object.

anthonyd

Comment 26

[Johnny Stenback (:jst)]

Oh, right, I somehow thought ToString was something we had added to Range to
make it work nicer in JS, but not! So given that, r=jst.

Comment 27

[anthonyd]

ok, i got a sr= from kin (given over irc) and a r= from jst.  The only thing 
left before this goes into is jsut to clear up a problem in the editrules code 
with joe francis.

anthonyd

Comment 28

[anthonyd]

Attachment: new proposed patch to fix rules code.

Comment 29

[anthonyd]

ok, could you take a look at my proposed patch for the rules code change?  It is 
in addition to the dom patch for range (the latest one i added).  I know I dont 
check in everything in the editrules code patch i just added, im just too tired 
right now to trim down the patch file.

anthonyd

Comment 30

[Joe Francis]

a=jfrancis

Comment 31

[anthonyd]

ok, im gonna check this in.

anthonyd

Comment 32

[anthonyd]

fix checked in
anthonyd

Comment 33

[rubydoo123]

catching up on verifications