Closed Bug 241698 Opened 18 years ago Closed 6 years ago

nsDirIndex::mLastModified not initialized properly

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jan.ruzicka, Assigned: sandor)

References

()

Details

(Whiteboard: [necko-backlog][good first bug])

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 241696
Summary: nsDirIndex::mLastModified not initialized properly → nsDirIndex::mLastModified not initialized properly
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/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: darin → nobody
QA Contact: benc → networking
Whiteboard: [necko-backlog][good first bug]
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
You can just write -1ll it is ok.
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.
Assignee: nobody → sandor
Status: NEW → ASSIGNED
A second version will follow that contains some minor enhancements too.
Attachment #8714873 - Flags: review?(mcmanus)
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.
Attachment #8714883 - Flags: review?(mcmanus)
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 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 <sandor@gecsey.net>
# 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 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 <sandor@gecsey.net>
# 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,
Attachment #8714883 - Attachment is obsolete: true
Attachment #8714883 - Flags: review?(mcmanus)
Attachment #8714893 - Flags: review?(mcmanus)
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...
(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.
Attachment #8714893 - Flags: review?(mcmanus) → review?(dd.mozilla)
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!
Attachment #8714873 - Flags: review?(mcmanus) → review?(dd.mozilla)
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.
Attachment #8714893 - Flags: review?(dd.mozilla) → feedback+
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.
Attachment #8714873 - Flags: review?(dd.mozilla)
Modified the patch V2 to use the suggested macro instead of a simple "if".
Attachment #8714893 - Attachment is obsolete: true
Attachment #8715379 - Flags: review?(dd.mozilla)
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
Attachment #8715379 - Flags: review?(dd.mozilla) → review+
Next, you will need a try run. Do you have access or should I do it for you?
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.
(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.
The try run look ok.
You can ask for check in now.
Status: ASSIGNED → NEW
Keywords: checkin-needed
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?
Status: NEW → ASSIGNED
Flags: needinfo?(sandor) → needinfo?(dd.mozilla)
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.
The failure does not look like something that can be cause by your patch. Request checkin again.
Flags: needinfo?(dd.mozilla)
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.
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/b2d240864875
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.