Bug 67805 – [Chromium] Crash in WebCore::DatabaseObserver

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Stephen Chenney]

http://code.google.com/p/chromium/issues/detail?id=88042

WebFrameImpl::fromFrame may return null, and DatabaseObserver::canEstablishDatabase does not check for this, leading to potential crashes.

[Stephen Chenney]

Created attachment 107549 [details]
Patch

[Michael Nordman]

this looks reasonable to me

i think changing the return value at the end to false is reasonable, but i don't know if there are chromium/webkit api users that are depending on the true return value at this time... are their any consumers that don't provide a permissionClient that this change would break?

also, i'm noticing that 'true' is the default return value for the allowDatabase method in WebPermissionsClient.h

maybe it's safer and more consistent to not change that return value at the end in this crash fixing patch

wdyt?

[Stephen Chenney]

I investigated further, looking for cases in the chromium code base where the WebPermissionsClient is set on the WebViewImpl. The only location is in chrome_render_view_observer, and the AllowDatabase setting for that ultimately comes from the browser's cookie settings. So it is certainly plausible that there exist other users of the database code that do not set any permissions client.

There is also database related code in WebWorkerClientImpl, and it defaults to true when there is no permissions client (and the permissions client always returns true, as far as I can tell).

Given this, and the default of true in the permissions client, I will change the return value.

[Stephen Chenney]

Created attachment 107663 [details]
Patch

[Michael Nordman]

fwiw lgtm (but i dont have review rights)

[Michael Nordman]

adding some webkit reviewers

[David Levin]

Comment on attachment 107663 [details]
Patch

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

Is it possible to test this?

Is it possible for webFrame->viewImpl() to be 0?

What about document->frame()?

> Source/WebKit/chromium/ChangeLog:10
> +        null frame and return false when the frame is null. Investiagted

typo: Investiagted

[Stephen Chenney]

I have no idea how to test that this resolves the crash, but I will look into it. At the same time I'll try to figure out why one thing may be null but not another.

[Stephen Chenney]

Is it possible to test this?
There was no repro case in the original crash report. It clearly depends on having a script execution context with a valid document (if you have the context you have the document) but an invalid frame. I'll try to repro with that in mind.

Is it possible for webFrame->viewImpl() to be 0?
webFrame->viewImpl() may be 0 if the frame is 0 or the page for the frame is 0. That means it should be checked too.

What about document->frame()?
document->frame() is probably the 0 object in this situation, but that does not need to be checked because WebFrameImpl::fromFrame(document->frame()) is happy to return 0 when it gets 0.

[Stephen Chenney]

Created attachment 108481 [details]
Patch

[Stephen Chenney]

I have no commit status at this time. Please do the honors for me.

[David Levin]

Comment on attachment 108481 [details]
Patch

Feel free to mark your patch as cq? if you'd like it marked cq+ :)

[WebKit Review Bot]

Comment on attachment 108481 [details]
Patch

Attachment 108481 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9818156

New failing tests:
svg/custom/svg-fonts-word-spacing.html

[David Levin]

Comment on attachment 108481 [details]
Patch

If at first you don't succeed... that test failure in svg is totally unrelated to this change.

[WebKit Review Bot]

Comment on attachment 108481 [details]
Patch

Clearing flags on attachment: 108481

Committed r95871: <http://trac.webkit.org/changeset/95871>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.