149320 - crash if invalid setStart is set on a Range

Status: RESOLVED FIXED

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

Description

[Dylan Schiemann]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc3)
Gecko/20020523
BuildID:    2002052306

setStart to an invalid offset for a Range.  Alert or appending of textNode of
Range causes crash.

Reproducible: Always
Steps to Reproduce:
1. createRange
2. setStart to invalid offset

Actual Results:  crash

Expected Results:  error

Comment 1

[Dylan Schiemann]

Attachment: testcase

Comment 2

[Dylan Schiemann]

Attachment: testcase

Added try-catch to testcase.  Should catch an INDEX_SIZE_ERR exception rather
than crashing.

Comment 3

[Adam Hauner]

With 2002060408/trunk/W2K using 1st testcase -> crash, TB7054045Q

Comment 4

[rubydoo123]

reassign to Kin

Comment 5

[jack.jia]

Attachment: a proposed patch

I add some condition to avoid crash for nil point, I think this patch can
match the reporter's need.

Comment 6

[jack.jia]

  who can review the patch for this bug.

Comment 7

[kinmoz]

Before checking in any content iterator changes, or getting a super review, I
would get a review from [e-mail] who wrote it.

That said, I believe there is an implicit assumption by the content iterator and
all backend users of range (like the editor) that the range will either be
un-positioned or contain valid containers and offsets.

I really think that we need to fix SetStart()/SetEnd()or the underlying
DoSetRange() function to validate indexes and containers ... I'm actually quite
surprised that it's not doing that already since the spec mentions specific
situations to check for.

Comment 8

[Joe Francis]

I agree with Kin.  Lets fix this in the right place.  Submitting patch soon.

Comment 9

[Joe Francis]

Attachment: patch to content/base/nsRange.{h,cpp}

This fixes it, and generates the out-of-bounds error expected in the second
testcase.  Kin, can you sr?

Comment 10

[Joe Francis]

The days of having a half dozen milestones out in front of us to divide bugs 
between seem to be gone, though I dont know why.  Lumping everything together as 
far out as I can.  I'll pull back things that I am working on as I go.

Comment 11

[kinmoz]

Comment on attachment 91184 [details] [diff] [review]
patch to content/base/nsRange.{h,cpp}

sr=[e-mail]

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 91184 [details] [diff] [review]
patch to content/base/nsRange.{h,cpp}

One comment only : DOM Comments are also CharacterData so I would prefer to see
them appear in the main 'if' case of GetNodeLength()...

Just in case we decide some day in the future to allow visibility of XML
comments.

r=glazman

Comment 13

[Joe Francis]

fix landed on trunk

Comment 14

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/54417ebbaea2