145994 - Crash in urildr.dll from some embedding clients

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Chak Nanga]

A crash occurs in urildr.dll when using certain embedding clients.

This started happening after the changes for
http://bugzilla.mozilla.org/show_bug.cgi?id=46856 went into the Mozila 1.0 branch.

A simple null check seems to prevent the crash, but, we need to make sure we're
not masking a bigger issue

Comment 1

[Chak Nanga]

Attachment: Patch which guards against null ptr access..

The patch is just to show where the crash occurs - we still need to make sure
that we're not masking a bigger issue at hand...thanks

Comment 2

[Jaime Rodriguez, Jr.]

Removing adt1.0.0, as this has not received reviews, nor had it been nominated
as nsbeta1.

Comment 3

[rpotts (gone)]

Attachment: more comprehensive patch...

Comment 4

[Chak Nanga]

Comment on attachment 84509 [details] [diff] [review]
more comprehensive patch...

r=chak

Comment 5

[Chak Nanga]

Cc:ing Alec for sr=

Comment 6

[Alec Flett]

Comment on attachment 84509 [details] [diff] [review]
more comprehensive patch...

should we put some assertions in there too, or is this an expected behavior?
sr=alecf on this, but lets add assertions if this is truly unexpected

Comment 7

[Chak Nanga]

adding adt1.0.0

Comment 8

[rpotts (gone)]

hey alec,

this behavior is 'unexpected' but i don't think it's an error :-)

basically, this situation arises when Stop() is called within a notification
callback from the DocLoader...  Stop() causes the internal list to be discarded
while it is being walked (higher up on the stack)...

i believe that we need to support this case.  in fact, i think that we need
further QA test cases to verify that all of the notifications are generated
correctly in this (and other unexpected) situations...

but, at this point i don't think we need an assert... and hopefully, at some
point the assert in nsVoidArray (saying that the element is out of bounds) will
go away too :-)

-- rick

Comment 9

[rpotts (gone)]

patch is checked into the trunk.

Comment 10

[Brendan Eich [:brendan]]

rick, you want SafeElementAt if you need the bounds-checking and don't want the
assertion.  ElementAt is the optimized-till-unsafe version.

/be

Comment 11

[Judson Valeski]

changing bug fields.

Comment 12

[Scott Collins]

Comment on attachment 84509 [details] [diff] [review]
more comprehensive patch...

a=scc (on behalf of drivers) for checkin to the mozilla1.0 branch

Comment 13

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for approval to checkin to the 1.0 branch. After,
checking in, please add the fixed1.0 keyword.

Comment 14

[rpotts (gone)]

Attachment: updated patch for the trunk using SafeElementAt(...)

Comment 15

[rpotts (gone)]

I'd like to check the last patch (using SafeElementAt) onto the trunk.  

The branch is safe with the original patch because no one is going to change the
implementation of nsVoidArray...

However, if this is changed on the trunk, then memory corruptions will occur!

-- rick

Comment 16

[Alec Flett]

Comment on attachment 84548 [details] [diff] [review]
updated patch for the trunk using SafeElementAt(...)

sr=alecf

Comment 17

[rpotts (gone)]

Attachment: oops... here's a cleaner patch.

Comment 18

[Alec Flett]

Comment on attachment 84549 [details] [diff] [review]
oops... here's a cleaner patch.

that works too :)
sr=alecf

Comment 19

[Dawn Endico]

according to bonsai, rpotts checked this in to the 1.0 branch last night.
05/21/2002 16:21

Comment 20

[rpotts (gone)]

yes.  i checked the first patch into both the branch and trunk.

i reopened the bug, so i could check in the changes from ElementAt() to 
SafeElementAt() into the trunk (to prevent future memory corruptions...).

-- rick

Comment 21

[Tony Wilson]

This may be related.  With build 2002060408 WinMe in the page 
http://www.allied-telesyn.com/allied/products/viewproduct.asp?category=1&id=77&country=218&lang=en
selecting the link "AT-2700 series datasheet" crashes the browser.

Tried twice, crashed twice so looks reproducible.

Comment 22

[Michael Dunn]

Rick-- What is the next step with this bug?

Comment 23

[Judson Valeski]

Chak, can you take a look see?

Comment 24

[Michael Buckland]

Discussed in topembed bug triage.  Minusing.

Comment 25

[Samir Gehani]

adt: nsbeta1-

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 84549 [details] [diff] [review]
oops... here's a cleaner patch.

r=jst, but this aint fixing no crash, nsVoidArray::ElementAt() is null safe,
even if it asserts...

Comment 27

[timeless]

mozilla/uriloader/base/nsDocLoader.cpp 	3.268