Bug 27698 – [V8] Database callbacks get wrong isolated world

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Adam Barth]

This looks broken from code inspection.  I haven't tested yet.

    Frame* frame = V8Proxy::retrieveFrame();
[...]
            callback = V8CustomSQLStatementCallback::create(args[2], frame);
[...]
    v8::Handle<v8::Context> context = V8Proxy::context(m_frame.get());

The callbacks should just hold onto a v8::Context.  Right now, they should get the v8::Context::GetCurrent() context.  (This is also wrong, but that's another bug.)

[Adam Barth]

I'd like the OwnHandle class from Bug 27533 before fixing this code.

[Adam Barth]

Bug 34726 shows how to do a this right.

[Dumitru Daniliuc]

Created attachment 52810 [details]
patch

Apologies for uploading a huge patch: it's really just 1 change applied to 5 different locations.

Adam, please review the changes to the V8 bindings.
Gavin, please review the changes to the JSC bindings + ScriptController.{h|cpp}.

[WebKit Review Bot]

Attachment 52810 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/js/ScriptController.cpp:172:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bindings/js/JSCustomSQLStatementErrorCallback.cpp:70:  One space before end of line comments  [whitespace/comments] [5]
WebCore/bindings/js/ScriptController.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/storage/SQLTransactionErrorCallback.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 52810 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1694006

[Dumitru Daniliuc]

Created attachment 52913 [details]
patch

[Eric Seidel (no email)]

Attachment 52913 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1609345

[WebKit Review Bot]

Attachment 52913 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1671181

[Adam Barth]

So much copy/paste code.  :(

+ RefPtr<Frame> m_frame

I though we decided we wanted to hold a ScriptExecutionContext instead of a m_frame.

[Jeremy Orlow]

(In reply to comment #9)
> So much copy/paste code.  :(
> 
> + RefPtr<Frame> m_frame
> 
> I though we decided we wanted to hold a ScriptExecutionContext instead of a
> m_frame.

I was the one talking about eliminating it, but it's fairly easy for Dumi to fix now.

Dumi: Take a look at IDBCallbacks.h + V8CustomIDBCallbacks.h  It's really not much harder to use the ActiveDOMObject stuff rather than storing a ref to the Frame (which is incorrect for several reasons) and it'll allow you to get notifications when the frame is going away (so you shouldn't do the callback...and should probably cancel the transaction).

Whether or not you do that, I'll be trying to unify our callback code at some point.

[Geoffrey Garen]

JSC looks good to me.

[Eric Seidel (no email)]

Comment on attachment 52913 [details]
patch

Looks like this breaks the mac and gtk builders.

[Dumitru Daniliuc]

Created attachment 53030 [details]
patch

Same patch. Changed some #includes to make them similar in all callbacks. Should build on all ports now.

[Adam Barth]

Comment on attachment 53030 [details]
patch

I feel like this patch highlights a ton of redundant code that might be profitably refactored or autogenerated.  Thanks for updating the patch.

[Dumitru Daniliuc]

Landed as r57530.