Closed Bug 62796 Opened 19 years ago Closed 19 years ago

Crash, bad exceptions following Range.detach()

Categories

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

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla0.9

People

(Reporter: david, Assigned: anthonyd)

Details

(Keywords: crash, Whiteboard: FIX IN HAND)

Attachments

(6 files)

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.
Keywords: correctness, crash
Attached file testcase
Reassigning to the owner of the Range code.
Assignee: jst → anthonyd
setting milestone, and priority.
anthonyd
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: --- → mozilla0.9
fix in hand, will attach for r= and sr= from kin and jst.

anthonyd
Whiteboard: FIX IN HAND
jst, i need an r= from you.

anthonyd
Patch looks good to me, r=jst
fix checked in.
anthonyd
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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?)
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The textbox problem showed up on my win32 build as well.  The changes have been
backed out.
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
Status: REOPENED → ASSIGNED
r=blake
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?
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?
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?
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
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
You might also want to fix the code in nsHTMLEditRules that I mentioned above.
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?
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
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.
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
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
a=jfrancis
ok, im gonna check this in.

anthonyd
fix checked in
anthonyd
Status: ASSIGNED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Component: DOM Level 2 → DOM Traversal-Range
catching up on verifications
Status: RESOLVED → VERIFIED
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.