If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash, bad exceptions following Range.detach()

VERIFIED FIXED in mozilla0.9

Status

()

Core
DOM: Core & HTML
P1
normal
VERIFIED FIXED
17 years ago
5 years ago

People

(Reporter: David Flanagan, Assigned: anthonyd)

Tracking

({crash})

Trunk
mozilla0.9
x86
Linux
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: FIX IN HAND)

Attachments

(6 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Updated

17 years ago
Keywords: correctness, crash
(Reporter)

Comment 1

17 years ago
Created attachment 20651 [details]
testcase
Reassigning to the owner of the Range code.
Assignee: jst → anthonyd
(Assignee)

Comment 3

17 years ago
setting milestone, and priority.
anthonyd
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 4

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

anthonyd
(Assignee)

Comment 5

17 years ago
Created attachment 21784 [details] [diff] [review]
patch for more range fixes
(Assignee)

Updated

17 years ago
Whiteboard: FIX IN HAND
(Assignee)

Comment 6

17 years ago
Created attachment 21786 [details] [diff] [review]
patch with kin's suggestions

Comment 7

17 years ago
sr=kin@netscape.com
(Assignee)

Comment 8

17 years ago
jst, i need an r= from you.

anthonyd
Patch looks good to me, r=jst
(Assignee)

Comment 10

17 years ago
fix checked in.
anthonyd
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 11

17 years ago
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 → ---

Comment 12

17 years ago
The textbox problem showed up on my win32 build as well.  The changes have been
backed out.
(Assignee)

Comment 13

17 years ago
Created attachment 22006 [details] [diff] [review]
new patch to handle input boxes problem
(Assignee)

Comment 14

17 years ago
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

Comment 15

17 years ago
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?
I just posted a (somewhat) related question to www-dom:
http://lists.w3.org/Archives/Public/www-dom/2001JanMar/0013.html
(Assignee)

Comment 20

17 years ago
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
(Assignee)

Comment 21

17 years ago
Created attachment 22113 [details] [diff] [review]
new patch to meet dom specs
(Assignee)

Comment 22

17 years ago
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?
(Assignee)

Comment 25

17 years ago
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.
(Assignee)

Comment 27

17 years ago
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
(Assignee)

Comment 28

17 years ago
Created attachment 22141 [details] [diff] [review]
new proposed patch to fix rules code.
(Assignee)

Comment 29

17 years ago
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

17 years ago
a=jfrancis
(Assignee)

Comment 31

17 years ago
ok, im gonna check this in.

anthonyd
(Assignee)

Comment 32

17 years ago
fix checked in
anthonyd
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
Component: DOM Level 2 → DOM Traversal-Range

Comment 33

15 years ago
catching up on verifications
Status: RESOLVED → VERIFIED
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.