Bug 67805 - [Chromium] Crash in WebCore::DatabaseObserver
Summary: [Chromium] Crash in WebCore::DatabaseObserver
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Stephen Chenney
Depends on:
Reported: 2011-09-08 14:28 PDT by Stephen Chenney
Modified: 2011-09-23 15:33 PDT (History)
6 users (show)

See Also:

Patch (1.78 KB, patch)
2011-09-15 14:11 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (1.65 KB, patch)
2011-09-16 09:19 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (1.86 KB, patch)
2011-09-23 08:45 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen Chenney 2011-09-08 14:28:24 PDT

WebFrameImpl::fromFrame may return null, and DatabaseObserver::canEstablishDatabase does not check for this, leading to potential crashes.
Comment 1 Stephen Chenney 2011-09-15 14:11:27 PDT
Created attachment 107549 [details]
Comment 2 Michael Nordman 2011-09-15 17:34:09 PDT
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

Comment 3 Stephen Chenney 2011-09-16 09:15:12 PDT
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.
Comment 4 Stephen Chenney 2011-09-16 09:19:46 PDT
Created attachment 107663 [details]
Comment 5 Michael Nordman 2011-09-16 13:43:32 PDT
fwiw lgtm (but i dont have review rights)
Comment 6 Michael Nordman 2011-09-19 13:44:34 PDT
adding some webkit reviewers
Comment 7 David Levin 2011-09-19 14:19:52 PDT
Comment on attachment 107663 [details]

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
Comment 8 Stephen Chenney 2011-09-21 08:51:46 PDT
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.
Comment 9 Stephen Chenney 2011-09-22 11:19:59 PDT
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.
Comment 10 Stephen Chenney 2011-09-23 08:45:14 PDT
Created attachment 108481 [details]
Comment 11 Stephen Chenney 2011-09-23 08:52:10 PDT
I have no commit status at this time. Please do the honors for me.
Comment 12 David Levin 2011-09-23 08:53:02 PDT
Comment on attachment 108481 [details]

Feel free to mark your patch as cq? if you'd like it marked cq+ :)
Comment 13 WebKit Review Bot 2011-09-23 09:07:53 PDT
Comment on attachment 108481 [details]

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

New failing tests:
Comment 14 David Levin 2011-09-23 11:10:28 PDT
Comment on attachment 108481 [details]

If at first you don't succeed... that test failure in svg is totally unrelated to this change.
Comment 15 WebKit Review Bot 2011-09-23 15:33:23 PDT
Comment on attachment 108481 [details]

Clearing flags on attachment: 108481

Committed r95871: <http://trac.webkit.org/changeset/95871>
Comment 16 WebKit Review Bot 2011-09-23 15:33:29 PDT
All reviewed patches have been landed.  Closing bug.