494103 - nsXMLContentSerializer UMR during update.mar tests

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Robert Sayre]

Attachment: valgrind log

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Gents: can we get some quick eyes on this?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think the relevant code is this:

1004   PRBool forceFormat, outputElementEnd;
1005   outputElementEnd = CheckElementEnd(content, forceFormat, aStr);
1006 
1007   nsIAtom *name = content->Tag();
1008 
1009   if ((mDoFormat || forceFormat) && !mPreLevel && !mDoRaw) {
1010     DecrIndentation(name);
1011   }
1012 
1013   if (!outputElementEnd) {
1014     PopNameSpaceDeclsFor(aElement);
1015     MaybeFlagNewlineForRootNode(aElement);
1016     return NS_OK;
1017   }

checkElementEnd doesn't set forceFormat if it returns false.  So if !outputElementEnd, then forceFormat is garbage.  The UMR will only happen if !mDoFormat, of course.

This code is newly added in bug 422403, which didn't land on branch.  So if I'm right that this is the UMR, no need to block 1.9.1 on it.

Laurent, can you check what the desired code flow is here?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking 3.5 as per comment 2; sayrer, were you evaluating against trunk or branch? If branch, please renominate!

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Against TM, if I read the log right.  Good call.

Comment 5

[Laurent Jouanneau]

I will check this issue this week-end. However, I don't understand what shows the valgrind log. And what does "UMR" mean ? :-)

Comment 6

[Robert Sayre]

Uninitialized Memory Read.

Valgrind is saying that the program is choosing the result of a conditional (if statement etc) based on initialized data. A simple case is this:

PRBool foo; 

if (foo) { // <-- foo is not initialized
  ...
}

In mozilla code, what usually happens is this:

PRBool foo; 
nsresult rv = DoSomething(&foo);

/* forget to check rv */

if (foo) { // <-- foo is not initialized if rv is a failure code
  ...
}

Comment 7

[Laurent Jouanneau]

Ok I see what append. Thanks Robert.

Comment 8

[Laurent Jouanneau]

Attachment: the patch [Checkin: Comment 11]

This patch initializes the "forceFormat" variables.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 379324 [details] [diff] [review]
the patch
[Checkin: Comment 11]

I don't think there's a need to init the forceFormat vars. Either way, r/sr=me.

Comment 10

[Laurent Jouanneau]

Comment on attachment 379324 [details] [diff] [review]
the patch
[Checkin: Comment 11]

this patch is only for mozilla-central. This problem doesn't appear in 1.9.1 branch (at least, not for this variables since they don't exist in 1.9.1)

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 379324 [details] [diff] [review]
the patch
[Checkin: Comment 11]


http://hg.mozilla.org/mozilla-central/rev/d9c96ff8b5ba