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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

[Ted Clancy [:tedders1]]

Attachment: patch-indexeddb-reportinternalerr.diff

Comment 2

[Ted Clancy [:tedders1]]

Attachment: patch-indexeddb-reportinternalerr.diff

Stylistic changes.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

[Ted Clancy [:tedders1]]

Attachment: patch-indexeddb-reportinternalerr.diff

Comment 5

[Ted Clancy [:tedders1]]

Attachment: patch-indexeddb-reportinternalerr.diff

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.)

Comment 6

[Jan Varga [:janv]]

Nit: ReportInternalError.h and ReportInternalError.cpp are missing in the patch :)

Comment 7

[Jan Varga [:janv]]

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 ?

Comment 8

[Ted Clancy [:tedders1]]

Attachment: patch-indexeddb-reportinternalerr.diff

Incorporating Jan's (:janv) suggestions. 

Also including ReportInternalError.h and ReportInternalError.cpp this time. Oops.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 10

[Ted Clancy [:tedders1]]

Attachment: bug-958359-fix.patch

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2bf80444eb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f2bf80444eb