Bug 226290 – Add logging for when SQLiteDatabase fails to close

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Sihui Liu]

...

[Sihui Liu]

Created attachment 429793 [details]
Patch

[Chris Dumez]

Comment on attachment 429793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429793&action=review

r=me with nits

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:238
> +            RELEASE_LOG_ERROR(SQLDatabase, "SQLiteDatabase::close: Failed to close database (%d) - %s", lastError(), lastErrorMsg());

Do you want a LOG_ERROR() too? I don't think you'll see the RELEASE_LOG_ERROR() on the bots' output, unless you get a sysdiagnose.

Also, you want to use %{public}s (assuming not privacy sensitive, %{private}s otherwise) on the string so the actually gets printed out in a RELEASE_LOG.

> Source/WebCore/platform/sql/SQLiteDatabase.h:62
> +    enum class ShouldSetErrorState { No, Yes };

enum class ShouldSetErrorState : bool { No, Yes };

[Sihui Liu]

Created attachment 429842 [details]
Patch for landing

[Sihui Liu]

Comment on attachment 429793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429793&action=review

>> Source/WebCore/platform/sql/SQLiteDatabase.cpp:238
>> +            RELEASE_LOG_ERROR(SQLDatabase, "SQLiteDatabase::close: Failed to close database (%d) - %s", lastError(), lastErrorMsg());
> 
> Do you want a LOG_ERROR() too? I don't think you'll see the RELEASE_LOG_ERROR() on the bots' output, unless you get a sysdiagnose.
> 
> Also, you want to use %{public}s (assuming not privacy sensitive, %{private}s otherwise) on the string so the actually gets printed out in a RELEASE_LOG.

The test I am investigating is an API test failed on release bot, so this is enough for me.
Will add %{public}s.

>> Source/WebCore/platform/sql/SQLiteDatabase.h:62
>> +    enum class ShouldSetErrorState { No, Yes };
> 
> enum class ShouldSetErrorState : bool { No, Yes };

Okay

[Chris Dumez]

(In reply to Sihui Liu from comment #4)
> Comment on attachment 429793 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429793&action=review
> 
> >> Source/WebCore/platform/sql/SQLiteDatabase.cpp:238
> >> +            RELEASE_LOG_ERROR(SQLDatabase, "SQLiteDatabase::close: Failed to close database (%d) - %s", lastError(), lastErrorMsg());
> > 
> > Do you want a LOG_ERROR() too? I don't think you'll see the RELEASE_LOG_ERROR() on the bots' output, unless you get a sysdiagnose.
> > 
> > Also, you want to use %{public}s (assuming not privacy sensitive, %{private}s otherwise) on the string so the actually gets printed out in a RELEASE_LOG.
> 
> The test I am investigating is an API test failed on release bot, so this is
> enough for me.
> Will add %{public}s.

Not sure what you mean by enough. Release logging doesn’t show in api tests output. Only way would be to get a sysdiagnose from the bots.

> 
> >> Source/WebCore/platform/sql/SQLiteDatabase.h:62
> >> +    enum class ShouldSetErrorState { No, Yes };
> > 
> > enum class ShouldSetErrorState : bool { No, Yes };
> 
> Okay

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> (In reply to Sihui Liu from comment #4)
> > Comment on attachment 429793 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=429793&action=review
> > 
> > >> Source/WebCore/platform/sql/SQLiteDatabase.cpp:238
> > >> +            RELEASE_LOG_ERROR(SQLDatabase, "SQLiteDatabase::close: Failed to close database (%d) - %s", lastError(), lastErrorMsg());
> > > 
> > > Do you want a LOG_ERROR() too? I don't think you'll see the RELEASE_LOG_ERROR() on the bots' output, unless you get a sysdiagnose.
> > > 
> > > Also, you want to use %{public}s (assuming not privacy sensitive, %{private}s otherwise) on the string so the actually gets printed out in a RELEASE_LOG.
> > 
> > The test I am investigating is an API test failed on release bot, so this is
> > enough for me.
> > Will add %{public}s.
> 
> Not sure what you mean by enough. Release logging doesn’t show in api tests
> output. Only way would be to get a sysdiagnose from the bots.

Maybe you want a temporary WTFLogAlways() since this is a release bot?

> > 
> > >> Source/WebCore/platform/sql/SQLiteDatabase.h:62
> > >> +    enum class ShouldSetErrorState { No, Yes };
> > > 
> > > enum class ShouldSetErrorState : bool { No, Yes };
> > 
> > Okay

[EWS]

Committed r278150 (238194@main): <https://commits.webkit.org/238194@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429842 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78551181>