Summary: | openDatabase() should not ignore the creationCallback if one is specified | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dumitru Daniliuc <dumi> | ||||||||||||||||
Component: | New Bugs | Assignee: | Dumitru Daniliuc <dumi> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, ap, dbates, dglazkov, dimich, ericu, ggaren, japhet, levin, michaeln, ossy, sam, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 35943 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Dumitru Daniliuc
2010-02-08 14:53:02 PST
Created attachment 49324 [details]
patch
Sam: can you please take a look at the JSC bindings?
Nate: can you please take a look at the V8 bindings?
Attachment 49324 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/js/JSDatabaseCallback.cpp:38: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #1) > Created an attachment (id=49324) [details] > patch > > Sam: can you please take a look at the JSC bindings? > Nate: can you please take a look at the V8 bindings? V8 bindings changes themselves look OK to me, though this patch should probably include Android build file changes? Created attachment 49460 [details]
patch
Fixed the style problem, and I think I made all the necessary changes to the Android build files, but I'm not 100% sure.
JSC bindings look good to me. Couple of comments/questions about the Database class. I haven't looked at the v8 or jsc bindings. > =================================================================== > --- WebCore/storage/Database.cpp (revision 55163) > +++ WebCore/storage/Database.cpp (working copy) > > -PassRefPtr<Database> Database::openDatabase(ScriptExecutionContext* context, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize, ExceptionCode& e) > +class DatabaseCreationCallbackTask : public ScriptExecutionContext::Task { > +public: > + static PassOwnPtr<DatabaseCreationCallbackTask> create(RefPtr<Database> database) Should the method parameter be a PassRefPtr? > + { > + return new DatabaseCreationCallbackTask(database); > + } > + > + virtual void performTask(ScriptExecutionContext*) > + { > + m_database->performCreationCallback(); > + } > + > +private: > + DatabaseCreationCallbackTask(RefPtr<Database> database) Ditto > + : m_database(database) > + { > + } > + > + RefPtr<Database> m_database; > +}; > + > > + // If it's a new database and a creation callback was provided, reset the expected > + // version to "" and schedule the creation callback. Because of some subtle String > + // implementation issues, we have to reset m_expectedVersion here instead of doing > + // it inside performOpenAndVerify() which is run on the DB thread. > + if (database.get() && database->isNew() && database->m_creationCallback.get()) { I don't think you need to test database.get() here, looks like it can't be null at this point. > + database->m_expectedVersion = ""; > + LOG(StorageAPI, "Scheduling DatabaseCreationCallbackTask for database %p\n", database.get()); > + database->m_scriptExecutionContext->postTask(DatabaseCreationCallbackTask::create(database)); > + } > + > return database; > } Created attachment 49669 [details]
patch
Addressed Michael's comments.
Comment on attachment 49669 [details]
patch
This looks pretty good, but the one problem I saw is that we might not be calling the database callback in the correct world. You're only saving a Frame* when you create the callback object. You need to save something that remembers which world we're in. There should be some example code for this pattern in the other code that makes callbacks.
Please add a test that the callback is called in the right world. You can find examples of these tests in the LayoutTests/http/tests/security/isolatedWorlds directory.
Created attachment 49768 [details]
patch
Added a test as requested by Adam. Hasn't made any other changes Adam mentioned as I could not find an example, but the test seems to indicate that they are not necessary (it passes in webkit/win and webkit/mac using JSC bindings and chromium/win using V8 bindings).
Ah, I think your test isn't testing quite the right thing. Can you try this variation? <!DOCTYPE html> <html> <body> <div id="console"></div> <script> function done() { if (window.layoutTestController) layoutTestController.notifyDone(); } function creationCallback(db) { alert("FAIL: Visible in isolated world."); done(); } document.body.foo = "FAIL: document.body.foo visible in isolated world."; if (window.layoutTestController) { layoutTestController.clearAllDatabases(); layoutTestController.dumpAsText(); layoutTestController.waitUntilDone(); layoutTestController.evaluateScriptInIsolatedWorld( 0, "function creationCallback(db)\n" + "{\n" + " alert(document.body.foo);\n" + " window.location='javascript:done()';\n" + "}\n" + "var db = openDatabase('OpenDatabaseCreationCallbackIsolatedWorld', '1.0', '', 1, creationCallback);"); } </script> </body> </html> Do the other Database related callbacks do the right thing with regard to isolated worlds? The transaction, statement, and error callbacks. > Do the other Database related callbacks do the right thing with regard to
> isolated worlds? The transaction, statement, and error callbacks.
Dunno. I don't mean to hold up this patch with this issue. We can deal with the isolated world interactions in a separate bug if you'd prefer.
Created attachment 49860 [details]
patch
Adam, I think I fixed the isolated worlds problem. Please take another look. The test passes in webkit/win (using JSC bindings) and chromium/win (using V8 bindings).
Comment on attachment 49860 [details]
patch
This looks great. Thanks!
Do we need to do the same thing for the other database callbacks?
(In reply to comment #14) > (From update of attachment 49860 [details]) > This looks great. Thanks! > > Do we need to do the same thing for the other database callbacks? There's a bug opened for that already: https://bugs.webkit.org/show_bug.cgi?id=27698. I will probably get to it in a couple of weeks. Landed as r55474. This change caused a test failure of fast/frames/sandboxed-iframe-storage.html across all of the bots, except the Qt Release bot (weird). The Qt Release bot is failing the test: storage/open-database-creation-callback.html. Reverted r55474 for reason: This patch broke fast/frames/sandboxed-iframe-storage.html Committed r55485: <http://trac.webkit.org/changeset/55485> Comment on attachment 49860 [details]
patch
Cleared r+ so it doesn't show up in a commit list.
Created attachment 50079 [details]
patch
Same patch, but fixing fast/frames/sandboxed-iframe-storage.html, and skipping one of the new tests on Qt since they don't seem to have LayoutTestController.evaluateScriptInIsolatedWorld() implemented.
Comment on attachment 50079 [details]
patch
ok
I forgot to add a couple of lines to the xcode project file, so the "mac" rectangle might turn red. I got it all figured out now and the patch builds and passes the layout tests on Mac. So I will commit it without uploading it, to save a review cycle. Re-landed as r55593. The patch has been reverted: http://trac.webkit.org/changeset/55635 It broke worker-cloneport.html on mac ports: see bug 35819. Created attachment 50461 [details] patch Same patch, minus the changes to Document::postTask() that were submitted as part of http://trac.webkit.org/changeset/55816. Comment on attachment 50461 [details]
patch
okiedokes
Re-re-landed as r55823. Guys, you forgot to re-re land my buildfix and adding the new test to Qt Skipped list. I did both of them: http://trac.webkit.org/changeset/55838 and http://trac.webkit.org/changeset/55839. |