Uninitialize variable potentially used.

RESOLVED FIXED

Status

()

RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: bratell, Unassigned)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
In nsGenericHTMLElement::CopyInnerTo the code at the end looks like:

  PRInt32 id;
  if (mDocument) {
    mDocument->GetAndIncrementContentID(&id);
  }

  aDst->SetContentID(id);  // <-- id will be uninitialized if mDocument is null.

It should probably be:

  if (mDocument) {
    PRInt32 id;
    mDocument->GetAndIncrementContentID(&id);
    aDst->SetContentID(id);
  }
Mass-reassigning bugs.
Assignee: jst → dom_bugs

Comment 2

16 years ago
Created attachment 121637 [details] [diff] [review]
fixes bug

fix for bug

Comment 3

16 years ago
Comment on attachment 121637 [details] [diff] [review]
fixes bug

Can I get R/SR/etc on this?
Attachment #121637 - Flags: superreview?
Attachment #121637 - Flags: review?

Comment 4

16 years ago
Comment on attachment 121637 [details] [diff] [review]
fixes bug

sounds like a descent idea in general, but I'm not sure whether the DOM slots
are in a sane state without SetContentID. So maybe just doing a 
PRInt32 id = 0;
may be the better fix.
peterv?
Attachment #121637 - Flags: superreview?(peterv)
Attachment #121637 - Flags: superreview?
Attachment #121637 - Flags: review?
Attachment #121637 - Flags: review+
Attachment #121637 - Flags: superreview?(peterv) → superreview+
if no document, should we be asserting, and calling

SetContentID(-1)?

and / or returning NS_ERROR_UNEXPECTED?

peterv, what do you think?
Created attachment 121840 [details] [diff] [review]
Default to -1, and get the document from mNodeInfo

No need to assert just because there's no document. This patch gets the
document from the elements nodeinfo, and as long as the document where the
element came from still exists, we'll get it, even if the element is no longer
part of the document. If we can't get a document, we'll set the content id to
-1, which IMO is what we should do. This will cause slots to be allocated, so
it's a bit expensive, but as long as we get the document from the nodeinfo,
that case will be so rare that I don't see how that could matter.
Comment on attachment 121840 [details] [diff] [review]
Default to -1, and get the document from mNodeInfo

I guess so, though I don't like passing negative numbers to a function that
takes a PRUint32.
Seth: it is not an error to not have a mDocument.
Attachment #121840 - Flags: superreview+
Um, right. s/-1/PR_UINT32_MAX/ on that patch.
Fixed as part of bug 215981.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.