Bug 103168 – [WK2] Add null check and early return code before accessing member of m_iconDatabaseImpl if IconDatabase is not created yet

• Status: UNCONFIRMED
• Component: WebKit2

[Changhyup Jwa]

If WebIconDatabase fails to create IconDatabase on WebIconDatabase::setDatabasePath(), m_iconDatabaseImpl references null.
So, it is necessary to check null and early return before accessing m_iconDatabaseImpl.

[Changhyup Jwa]

Created attachment 175847 [details]
Patch

[Andreas Kling]

Comment on attachment 175847 [details]
Patch

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

> Source/WebKit2/UIProcess/WebIconDatabase.cpp:74
>      m_iconDatabaseImpl =  IconDatabase::create();
> +    if (!m_iconDatabaseImpl) {

How can this be null?

[Alexey Proskuryakov]

Failing to create an icon database is pretty much a fatal condition for a browser.

[Changhyup Jwa]

Comment on attachment 175847 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebIconDatabase.cpp:74
>> +    if (!m_iconDatabaseImpl) {
> 
> How can this be null?

Yes Andreas, you are right. It cannot be null. I'll make another patch immediately.

[Changhyup Jwa]

Created attachment 175905 [details]
Patch

[Brent Fulgham]

Comment on attachment 175905 [details]
Patch

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

> Source/WebKit2/UIProcess/WebIconDatabase.cpp:267
> +    if (!m_webContext || m_iconDatabaseImpl)

Why do we exit early if m_iconDarabaseImpl exists?

[Changhyup Jwa]

Comment on attachment 175905 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebIconDatabase.cpp:267
>> +    if (!m_webContext || m_iconDatabaseImpl)
> 
> Why do we exit early if m_iconDarabaseImpl exists?

Thanks for your comment Brent, apologize for my mistake.

[Changhyup Jwa]

Created attachment 175919 [details]
Patch

[Anders Carlsson]

Comment on attachment 175919 [details]
Patch

I agree with Alexey here, m_iconDatabaseImpl should never be null. If it is for some reason, then that underlying issue should be fixed instead of papering over it with random null checks.

[Changhyup Jwa]

(In reply to comment #9)
> (From update of attachment 175919 [details])
> I agree with Alexey here, m_iconDatabaseImpl should never be null. If it is for some reason, then that underlying issue should be fixed instead of papering over it with random null checks.

Thanks for your review, Anders :)