241698 - nsDirIndex::mLastModified not initialized properly

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jan Ruzicka]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040425
Build Identifier: 

the  nsDirIndex::mLastModified is defined as PRInt64.
As 64 bit integer it should be manipulated by LL_ macros.

For example 
nsDirIndex.cpp
instead of :
       mLastModified(-1)

it should be :
       mLastModified(LL_INIT(0, -1))

references:
http://lxr.mozilla.org/mozilla/source/netwerk/streamconv/converters/nsDirIndex.h#58
http://lxr.mozilla.org/mozilla/source/netwerk/streamconv/converters/nsDirIndex.cpp#49

http://lxr.mozilla.org/mozilla/source/netwerk/streamconv/converters/nsDirIndex.cpp

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 2

[Sándor Gecsey]

Hi,

I'm a newcomer seeking for the first bug to solve so I stumbled upon this one. In fact I tried out the solution suggested in the description and it works (well, no wonder). But I'm not sure the proposal is the best solution. -1 is a 32 bit integer so it's definitely wrong but:

The documentation for nsIDirIndex::lastModified says that "-1 means unknown - this is valid, because there were no ftp servers in 1969" but 0x00000000ffffffff is not -1 (0xffffffffffffffff because it's a 2's complement). My questions are:
 - should be LL_INIT(-1, -1) instead, or
 - is -1LL also allowed (even older compiler should support this literal but I don't know whether ancient ones too need to be considered for Firefox)?

Anyway I feel a bit "stupid" sitting over this kind of a "bug" and I'm also wondering how this easy thing could live 12 years but it was perfect for quickly trying out the build process of Firefox. So if someone still cares about this bug please answer my question. I'd rather write -1LL because it's the clearest solution.

Thx,
Sándor

Comment 3

[Dragana Damjanovic [:dragana]]

You can just write -1ll it is ok.

Comment 4

[Sándor Gecsey]

Ok, thank you. I'll prepare a patch. Is it necessary to assign the bug to me? If it is so please do that because I assume I don't have the permission to do it currently.

Comment 5

[Sándor Gecsey]

Attachment: solution v1 that contains only that fix this bug

A second version will follow that contains some minor enhancements too.

Comment 6

[Sándor Gecsey]

Attachment: solution v2 that fixes the bug and contains some minor enhancements too

I don't know how important it is to avoid possible nullptr crashes in properties of this class but I added them to this patch where they were missing.

Comment 7

[Sándor Gecsey]

So I finally attached 2 patches as described above. I hope they are in the correct format and I didn't do any mistake following the "patch howto".

Comment 8

[Sándor Gecsey]

Comment on attachment 8714883 [details] [diff] [review]
solution v2 that fixes the bug and contains some minor enhancements too

# HG changeset patch
# User Sándor Gecsey <[e-mail]>
# Parent  d07dbd40dcd209124149f331f60dd55c8da33fbe
Bug 241698 - Fixed init and use of nsDirIndex::mLastModified (-1LL) + built in nullptr checks where they were missing.
Also unified the opening brackets in method bodies because they differed.

diff --git a/netwerk/streamconv/converters/nsDirIndex.cpp b/netwerk/streamconv/converters/nsDirIndex.cpp
--- a/netwerk/streamconv/converters/nsDirIndex.cpp
+++ b/netwerk/streamconv/converters/nsDirIndex.cpp
@@ -5,17 +5,18 @@
 
 #include "nsDirIndex.h"
 
 NS_IMPL_ISUPPORTS(nsDirIndex,
                   nsIDirIndex)
 
 nsDirIndex::nsDirIndex() : mType(TYPE_UNKNOWN),
                            mSize(UINT64_MAX),
-                           mLastModified(-1) {
+                           mLastModified(-1LL)
+{
 }
 
 nsDirIndex::~nsDirIndex() {}
 
 NS_IMETHODIMP
 nsDirIndex::GetType(uint32_t* aType)
 {
   if (!aType) {
@@ -29,56 +30,74 @@ nsDirIndex::GetType(uint32_t* aType)
 NS_IMETHODIMP
 nsDirIndex::SetType(uint32_t aType)
 {
   mType = aType;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::GetContentType(char* *aContentType) {
+nsDirIndex::GetContentType(char* *aContentType)
+{
+  if (!aContentType) {
+    return NS_ERROR_NULL_POINTER;
+  }
+
   *aContentType = ToNewCString(mContentType);
   if (!*aContentType)
     return NS_ERROR_OUT_OF_MEMORY;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::SetContentType(const char* aContentType) {
+nsDirIndex::SetContentType(const char* aContentType)
+{
   mContentType = aContentType;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::GetLocation(char* *aLocation) {
+nsDirIndex::GetLocation(char* *aLocation)
+{
+  if (!aLocation) {
+    return NS_ERROR_NULL_POINTER;
+  }
+
   *aLocation = ToNewCString(mLocation);
   if (!*aLocation)
     return NS_ERROR_OUT_OF_MEMORY;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::SetLocation(const char* aLocation) {
+nsDirIndex::SetLocation(const char* aLocation)
+{
   mLocation = aLocation;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::GetDescription(char16_t* *aDescription) {
+nsDirIndex::GetDescription(char16_t* *aDescription)
+{
+  if (!aDescription) {
+    return NS_ERROR_NULL_POINTER;
+  }
+
   *aDescription = ToNewUnicode(mDescription);
   if (!*aDescription)
     return NS_ERROR_OUT_OF_MEMORY;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::SetDescription(const char16_t* aDescription) {
+nsDirIndex::SetDescription(const char16_t* aDescription)
+{
   mDescription.Assign(aDescription);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDirIndex::GetSize(int64_t* aSize)
 {
   if (!aSize) {
diff --git a/netwerk/streamconv/converters/nsIndexedToHTML.cpp b/netwerk/streamconv/converters/nsIndexedToHTML.cpp
--- a/netwerk/streamconv/converters/nsIndexedToHTML.cpp
+++ b/netwerk/streamconv/converters/nsIndexedToHTML.cpp
@@ -822,17 +822,17 @@ nsIndexedToHTML::OnIndexAvailable(nsIReq
             pushBuffer.Append('>');
         }
     }
     pushBuffer.AppendLiteral("</td>\n <td");
 
     PRTime t;
     aIndex->GetLastModified(&t);
 
-    if (t == -1) {
+    if (t == -1LL) {
         pushBuffer.AppendLiteral("></td>\n <td>");
     } else {
         pushBuffer.AppendLiteral(" sortable-data=\"");
         pushBuffer.AppendInt(static_cast<int64_t>(t));
         pushBuffer.AppendLiteral("\">");
         nsAutoString formatted;
         mDateTime->FormatPRTime(nullptr,
                                 kDateFormatShort,

Comment 9

[Sándor Gecsey]

Comment on attachment 8714883 [details] [diff] [review]
solution v2 that fixes the bug and contains some minor enhancements too

# HG changeset patch
# User Sándor Gecsey <[e-mail]>
# Parent  d07dbd40dcd209124149f331f60dd55c8da33fbe
Bug 241698 - Fixed init and use of nsDirIndex::mLastModified (-1LL) + built in nullptr checks where they were missing.
Also unified the opening brackets in method bodies because they differed.

diff --git a/netwerk/streamconv/converters/nsDirIndex.cpp b/netwerk/streamconv/converters/nsDirIndex.cpp
--- a/netwerk/streamconv/converters/nsDirIndex.cpp
+++ b/netwerk/streamconv/converters/nsDirIndex.cpp
@@ -5,17 +5,18 @@
 
 #include "nsDirIndex.h"
 
 NS_IMPL_ISUPPORTS(nsDirIndex,
                   nsIDirIndex)
 
 nsDirIndex::nsDirIndex() : mType(TYPE_UNKNOWN),
                            mSize(UINT64_MAX),
-                           mLastModified(-1) {
+                           mLastModified(-1LL)
+{
 }
 
 nsDirIndex::~nsDirIndex() {}
 
 NS_IMETHODIMP
 nsDirIndex::GetType(uint32_t* aType)
 {
   if (!aType) {
@@ -29,56 +30,74 @@ nsDirIndex::GetType(uint32_t* aType)
 NS_IMETHODIMP
 nsDirIndex::SetType(uint32_t aType)
 {
   mType = aType;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::GetContentType(char* *aContentType) {
+nsDirIndex::GetContentType(char* *aContentType)
+{
+  if (!aContentType) {
+    return NS_ERROR_NULL_POINTER;
+  }
+
   *aContentType = ToNewCString(mContentType);
   if (!*aContentType)
     return NS_ERROR_OUT_OF_MEMORY;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::SetContentType(const char* aContentType) {
+nsDirIndex::SetContentType(const char* aContentType)
+{
   mContentType = aContentType;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::GetLocation(char* *aLocation) {
+nsDirIndex::GetLocation(char* *aLocation)
+{
+  if (!aLocation) {
+    return NS_ERROR_NULL_POINTER;
+  }
+
   *aLocation = ToNewCString(mLocation);
   if (!*aLocation)
     return NS_ERROR_OUT_OF_MEMORY;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::SetLocation(const char* aLocation) {
+nsDirIndex::SetLocation(const char* aLocation)
+{
   mLocation = aLocation;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::GetDescription(char16_t* *aDescription) {
+nsDirIndex::GetDescription(char16_t* *aDescription)
+{
+  if (!aDescription) {
+    return NS_ERROR_NULL_POINTER;
+  }
+
   *aDescription = ToNewUnicode(mDescription);
   if (!*aDescription)
     return NS_ERROR_OUT_OF_MEMORY;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsDirIndex::SetDescription(const char16_t* aDescription) {
+nsDirIndex::SetDescription(const char16_t* aDescription)
+{
   mDescription.Assign(aDescription);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDirIndex::GetSize(int64_t* aSize)
 {
   if (!aSize) {
diff --git a/netwerk/streamconv/converters/nsIndexedToHTML.cpp b/netwerk/streamconv/converters/nsIndexedToHTML.cpp
--- a/netwerk/streamconv/converters/nsIndexedToHTML.cpp
+++ b/netwerk/streamconv/converters/nsIndexedToHTML.cpp
@@ -822,17 +822,17 @@ nsIndexedToHTML::OnIndexAvailable(nsIReq
             pushBuffer.Append('>');
         }
     }
     pushBuffer.AppendLiteral("</td>\n <td");
 
     PRTime t;
     aIndex->GetLastModified(&t);
 
-    if (t == -1) {
+    if (t == -1LL) {
         pushBuffer.AppendLiteral("></td>\n <td>");
     } else {
         pushBuffer.AppendLiteral(" sortable-data=\"");
         pushBuffer.AppendInt(static_cast<int64_t>(t));
         pushBuffer.AppendLiteral("\">");
         nsAutoString formatted;
         mDateTime->FormatPRTime(nullptr,
                                 kDateFormatShort,

Comment 10

[Sándor Gecsey]

Attachment: final solution v2 that fixes the bug and contains some minor enhancements too

Comment 11

[Sándor Gecsey]

Sorry for the multiple updates but this is the first time I'm using bugzilla and I had to update the second patch and it took a while to figure out how that can be made...

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to Sándor Gecsey from comment #11)
> Sorry for the multiple updates but this is the first time I'm using bugzilla
> and I had to update the second patch and it took a while to figure out how
> that can be made...

No problem. The patches look fine. Before pushing it to the repo we add also r=name of the reviewer at the end of the comment.

You can add that later it is not a problem, I usually add it from the beginning so I do not need to update patch.

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 8714873 [details] [diff] [review]
solution v1 that contains only that fix this bug

Review of attachment 8714873 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for the patches!

Comment 14

[Dragana Damjanovic [:dragana]]

Comment on attachment 8714893 [details] [diff] [review]
final solution v2 that fixes the bug and contains some minor enhancements too

Review of attachment 8714893 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.

::: netwerk/streamconv/converters/nsDirIndex.cpp
@@ +38,5 @@
> +nsDirIndex::GetContentType(char* *aContentType)
> +{
> +  if (!aContentType) {
> +    return NS_ERROR_NULL_POINTER;
> +  }

This code is used only 2 places therefore this pointer were not checked, but we should fix this.

There is NS_ENSURE_ARG_POINTER you should use here.

Comment 15

[Dragana Damjanovic [:dragana]]

Comment on attachment 8714873 [details] [diff] [review]
solution v1 that contains only that fix this bug

Review of attachment 8714873 [details] [diff] [review]:
-----------------------------------------------------------------

We can use the other patch.

Comment 16

[Sándor Gecsey]

Attachment: final solution v2 that fixes the bug and contains some minor enhancements too

Modified the patch V2 to use the suggested macro instead of a simple "if".

Comment 17

[Dragana Damjanovic [:dragana]]

Comment on attachment 8715379 [details] [diff] [review]
final solution v2 that fixes the bug and contains some minor enhancements too

Review of attachment 8715379 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 18

[Dragana Damjanovic [:dragana]]

Next, you will need a try run. Do you have access or should I do it for you?

Comment 19

[Sándor Gecsey]

Currently I don't have access to the try server (or I'm not aware of it) and if I understood the wiki correctly one needs to be a Level 1 member for that. And I don't think I'm that.

So start the try run please!

Anyway I compiled the sources locally of course and I didn't find any automated test testing this tiny class.

Comment 20

[Dragana Damjanovic [:dragana]]

(In reply to Sándor Gecsey from comment #19)
> Currently I don't have access to the try server (or I'm not aware of it) and
> if I understood the wiki correctly one needs to be a Level 1 member for
> that. And I don't think I'm that.
> 
> So start the try run please!
> 
> Anyway I compiled the sources locally of course and I didn't find any
> automated test testing this tiny class.


It is use by html directory parser, I am not sure if there is a test for that but it is good practice to check it on try.

Comment 21

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5f621c22736

Comment 22

[Dragana Damjanovic [:dragana]]

The try run look ok.
You can ask for check in now.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4eab8351973c

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa076a770ac0 for being a possible cause of build bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=21169623&repo=mozilla-inbound

Comment 25

[Sándor Gecsey]

I tried to check the raw log but I couldn't find any of the 2 files I modified (nsDirIndex.cpp and nsIndexedToHTML.cpp). And without the concrete error message it is hard to investigate the issue. All I could find was this:

14:47:58     INFO -  Assertion failure: containsPC(pc), at /builds/slave/m-in-m64-d-0000000000000000000/build/src/js/src/jsscript.h:1288
14:48:39     INFO -  Traceback (most recent call last):
14:48:39     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 410, in <module>
14:48:39     INFO -      main()
14:48:39     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 404, in main
14:48:39     INFO -      args.source, gre_path, base)
14:48:39     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 161, in precompile_cache
14:48:39     INFO -      errors.fatal('Error while running startup cache precompilation')
14:48:39     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/python/mozbuild/mozpack/errors.py", line 103, in fatal
14:48:39     INFO -      self._handle(self.FATAL, msg)
14:48:39     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/python/mozbuild/mozpack/errors.py", line 98, in _handle
14:48:39     INFO -      raise ErrorMessage(msg)
14:48:39     INFO -  mozpack.errors.ErrorMessage: Error: Error while running startup cache precompilation
14:48:39     INFO -  make[4]: *** [stage-package] Error 1
14:48:39     INFO -  make[3]: *** [make-package] Error 2
14:48:39     INFO -  make[2]: *** [default] Error 2
14:48:39     INFO -  make[1]: *** [package] Error 2
14:48:39     INFO -  make: *** [automation/package] Error 2
14:48:39     INFO -  274 compiler warnings present.
14:48:44     INFO -  Notification center failed: Install terminal-notifier to get a notification when the build finishes.
14:48:44    ERROR - Return code: 2
14:48:44  WARNING - setting return code to 2
14:48:44    FATAL - 'mach build' did not run successfully. Please check log for errors.
14:48:44    FATAL - Running post_fatal callback...
14:48:44    FATAL - Exiting -1

So the build must have failed, right?

Or could this failure be the issue?

14:47:57     INFO -  [45958] WARNING: NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED) failed with result 0x80004005: file /builds/slave/m-in-m64-d-0000000000000000000/build/src/extensions/cookie/nsPermissionManager.cpp, line 822

But that rather seems to be a runtime assert failure.

All in all the information available for me is not enough. Could anyone (maybe Dragana?) help me where can I find more detailed logs?

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm almost positive these failures were from one of the other patches that got pushed along with this. You should be able to just request checkin-needed again for it to reland.

Comment 27

[Dragana Damjanovic [:dragana]]

The failure does not look like something that can be cause by your patch. Request checkin again.

Comment 28

[Sándor Gecsey]

Updated my local workspace, recompiled it (on Linux) with success. I also think it is very unlikely that this patch causes build issues so I request checkin again.

Comment 29

[Phil Ringnalda (:philor)]

FWIW (very little, other than that you might see the same horrid failure again sometime), that failure, the key part being "Error while running startup cache precompilation", means that the build compiled, but when it was run to prefill the mysterious startup cache which does some preprocessing of XUL and XBL in a way understood by four people, three of whom are gone, it crashed. Horrid because you generally get absolutely no hint about how it crashed, that one with a visible assertion failure is far better than most.

Anyway, let's get you landed alone and in a quiet time, when we'll be less likely to back you out over someone else's bustage.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d240864875

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b2d240864875