Closed Bug 958359 Opened 6 years ago Closed 6 years ago

indexedDB should log a message to the console when it returns NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR

Categories

(Core :: Storage: IndexedDB, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)

People

(Reporter: tedders1, Assigned: tedders1)

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 5 obsolete files)

No description provided.
QA Contact: tclancy
Assignee: nobody → tclancy
Status: NEW → ASSIGNED
QA Contact: tclancy
Attachment #8358104 - Flags: review?(bent.mozilla)
Stylistic changes.
Attachment #8358104 - Attachment is obsolete: true
Attachment #8358104 - Flags: review?(bent.mozilla)
Attachment #8359505 - Flags: review?(bent.mozilla)
Comment on attachment 8359505 [details] [diff] [review]
patch-indexeddb-reportinternalerr.diff

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

This looks really good! I'd like to see one more version with these comments addressed:

::: dom/indexedDB/IDBTransaction.cpp
@@ +832,4 @@
>                                   NS_LITERAL_STRING(COMPLETE_EVT_STR),
>                                   eDoesNotBubble, eNotCancelable);
>      }
> +    INDEXEDDB_ENSURE_TRUE_WITH_REPORT(event, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

This line (and maybe some others?) exceeds 80 chars, you can wrap like:

  INDEXEDDB_ENSURE_TRUE_WITH_REPORT(event,
                                    NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

Please check the other replacements and wrap if needed.

::: dom/indexedDB/ReportInternalError.cpp
@@ +1,1 @@
> +#include "ReportInternalError.h"

This file needs a license header as well.

@@ +4,5 @@
> +#include "nsPrintfCString.h"
> +
> +BEGIN_INDEXEDDB_NAMESPACE
> +
> +void 

Nit: Trailing space

@@ +13,5 @@
> +    if (*p == '/' && *(p + 1)) {
> +      aFile = p + 1;
> +    }
> +  }
> +  

Nit: Trailing spaces

@@ +14,5 @@
> +      aFile = p + 1;
> +    }
> +  }
> +  
> +  nsContentUtils::LogSimpleConsoleError(NS_ConvertUTF8toUTF16(nsPrintfCString("IndexedDB %s: %s:%d", aStr, aFile, aLine)), "indexedDb");

s/%d/%lu/ when you change to uint32_t

Also, s/"indexedDb"/"indexedDB"/, though I kinda doubt it matters.

::: dom/indexedDB/ReportInternalError.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_indexeddb_reportinternalerror_h__
> +#define mozilla_dom_indexeddb_reportinternalerror_h__
> +
> +#include "nsContentUtils.h"

It doesn't look like this is needed in the header. Though you should #include "nsDebug.h" for the other things you're using.

@@ +12,5 @@
> +#include "IndexedDatabase.h"
> +
> +#define INDEXEDDB_WARNING_WITH_REPORT(x)                                       \
> +  NS_WARNING(x);                                                               \
> +  mozilla::dom::indexedDB::ReportInternalError(__FILE__, __LINE__, x)

The ENSURE_ macros below both call ReportInternalError() before NS_WARNING. I think we should do that here too, so let's switch the order.

@@ +14,5 @@
> +#define INDEXEDDB_WARNING_WITH_REPORT(x)                                       \
> +  NS_WARNING(x);                                                               \
> +  mozilla::dom::indexedDB::ReportInternalError(__FILE__, __LINE__, x)
> +
> +#define INDEXEDDB_ERROR_WITH_REPORT(x)                                         \

I know you're just preserving the existing code, but this one should probably be removed. One place it's used (CreateSuccessEvent) should be switched to INDEXEDDB_WARNING_WITH_REPORT, and the other place (DatabaseInfo::Put) is now infallible so that's just dead code.

@@ +19,5 @@
> +  NS_ERROR(x);                                                                 \
> +  mozilla::dom::indexedDB::ReportInternalError(__FILE__, __LINE__, x)
> +
> +#define INDEXEDDB_NOTREACHED_WITH_REPORT(x)                                    \
> +  NS_NOTREACHED(x);                                                            \

This one, too, should probably be removed. Just replace the existing NS_NOTREACHED with MOZ_CRASH (and then remove any code that follows, like 'return  NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR'). That is newer and much more fatal.

@@ +50,5 @@
> +
> +
> +BEGIN_INDEXEDDB_NAMESPACE
> +
> +void 

Nit: You've got a trailing space here.

@@ +51,5 @@
> +
> +BEGIN_INDEXEDDB_NAMESPACE
> +
> +void 
> +ReportInternalError(const char* aFile, int aLine, const char* aStr);

I think we want uint32_t here for aLine. It shouldn't ever be negative.
Attachment #8359505 - Flags: review?(bent.mozilla)
Attachment #8363391 - Flags: review?(bent.mozilla)
Made the modifications suggested by Ben above.

(With one slight change. Rather than "%lu", I used the PRIu32 macro from inttypes.h as the format specifier for the uint32_t argument to printf.)
Attachment #8359505 - Attachment is obsolete: true
Attachment #8363391 - Attachment is obsolete: true
Attachment #8363391 - Flags: review?(bent.mozilla)
Attachment #8363395 - Flags: review?(bent.mozilla)
Nit: ReportInternalError.h and ReportInternalError.cpp are missing in the patch :)
I also wonder if we should shorten the prefix, INDEXEDDB_ -> IDB_
and remove the suffix _WITH_REPORT, or you plan to add a non reporting version in future ?
Incorporating Jan's (:janv) suggestions. 

Also including ReportInternalError.h and ReportInternalError.cpp this time. Oops.
Attachment #8363395 - Attachment is obsolete: true
Attachment #8363395 - Flags: review?(bent.mozilla)
Attachment #8363997 - Flags: review?(bent.mozilla)
Whiteboard: [systemsfe]
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Comment on attachment 8363997 [details] [diff] [review]
patch-indexeddb-reportinternalerr.diff

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

This looks good, r=me with these comments addressed:

::: dom/indexedDB/ReportInternalError.cpp
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "ReportInternalError.h"
> +
> +#include <inttypes.h>

In general this should be "mozilla/IntegerPrintfMacros.h" to make windows happy.

However, since you're using nsPrintfCString I don't think you don't need this at all, see below.

@@ +13,5 @@
> +
> +BEGIN_INDEXEDDB_NAMESPACE
> +
> +void
> +ReportInternalError(const char* aFile, uint32_t aLine, const char *aStr)

Nit: * on the left for aStr

@@ +16,5 @@
> +void
> +ReportInternalError(const char* aFile, uint32_t aLine, const char *aStr)
> +{
> +  // Get leaf of file path
> +  for (const char *p = aFile; *p; ++p) {

Nit: * on the left, so |const char* p = aFile|

@@ +24,5 @@
> +  }
> +
> +  nsContentUtils::LogSimpleConsoleError(
> +    NS_ConvertUTF8toUTF16(nsPrintfCString(
> +                          "IndexedDB %s: %s:" PRIu32, aStr, aFile, aLine)), 

Since this is our own nsPrintfCString you can just use these format specifiers:

  http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prprf.h#10

So here just '%lu' for the line number

Also, nit: trailing whitespace
Attachment #8363997 - Flags: review?(bent.mozilla) → review+
Attachment #8363997 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [systemsfe] → [systemsfe] [b2g-inbound]
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2bf80444eb
Keywords: checkin-needed
Whiteboard: [systemsfe] [b2g-inbound] → [systemsfe]
https://hg.mozilla.org/mozilla-central/rev/9f2bf80444eb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.