Closed Bug 145994 Opened 22 years ago Closed 20 years ago

Crash in urildr.dll from some embedding clients

Categories

(Core Graveyard :: Embedding: APIs, defect)

x86
Windows 2000
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: chak, Assigned: chak)

References

Details

(Keywords: crash, topembed-)

Attachments

(2 files, 2 obsolete files)

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
Keywords: adt1.0.0, topembed+
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
Keywords: crash
Removing adt1.0.0, as this has not received reviews, nor had it been nominated
as nsbeta1.
Keywords: adt1.0.0nsbeta1
Whiteboard: [adt2 RTM]
Attachment #84453 - Attachment is obsolete: true
Comment on attachment 84509 [details] [diff] [review]
more comprehensive patch...

r=chak
Attachment #84509 - Flags: review+
Cc:ing Alec for sr=
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
Attachment #84509 - Flags: superreview+
adding adt1.0.0
Keywords: adt1.0.0
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

Keywords: adt1.0.0
patch is checked into the trunk.
rick, you want SafeElementAt if you need the bounds-checking and don't want the
assertion.  ElementAt is the optimized-till-unsafe version.

/be
changing bug fields.
Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: adt1.0.0
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.0
Comment on attachment 84509 [details] [diff] [review]
more comprehensive patch...

a=scc (on behalf of drivers) for checkin to the mozilla1.0 branch
Attachment #84509 - Flags: approval+
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.
Keywords: adt1.0.0adt1.0.0+
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: fixed1.0.0
Comment on attachment 84548 [details] [diff] [review]
updated patch for the trunk using SafeElementAt(...)

sr=alecf
Attachment #84548 - Flags: superreview+
Attachment #84548 - Attachment is obsolete: true
Comment on attachment 84549 [details] [diff] [review]
oops... here's a cleaner patch.

that works too :)
sr=alecf
Attachment #84549 - Flags: superreview+
according to bonsai, rpotts checked this in to the 1.0 branch last night.
05/21/2002 16:21
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
Blocks: 143047
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.
Rick-- What is the next step with this bug?
Target Milestone: mozilla1.0 → ---
Chak, can you take a look see?
Assignee: rpotts → chak
Status: REOPENED → NEW
Discussed in topembed bug triage.  Minusing.
Keywords: topembed+topembed-
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
Whiteboard: [adt2 RTM]
Severity: blocker → critical
Attachment #84549 - Flags: review?(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...
Attachment #84549 - Flags: review?(jst) → review+
mozilla/uriloader/base/nsDocLoader.cpp 	3.268
Status: NEW → RESOLVED
Closed: 22 years ago20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: