Bug 27698

Summary: [V8] Database callbacks get wrong isolated world
Product: WebKit Reporter: Adam Barth <abarth>
Component: JavaScriptCoreAssignee: Dumitru Daniliuc <dumi>
Status: RESOLVED FIXED    
Severity: Normal CC: ager, barraclough, dglazkov, dumi, eric, fishd, ggaren, gustavo, jorlow, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 27533    
Bug Blocks:    
Attachments:
Description Flags
patch
dumi: commit-queue-
patch
eric: review-, dumi: commit-queue-
patch abarth: review+, dumi: commit-queue-

Description Adam Barth 2009-07-27 01:36:02 PDT
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.)
Comment 1 Adam Barth 2009-07-27 01:37:03 PDT
I'd like the OwnHandle class from Bug 27533 before fixing this code.
Comment 2 Adam Barth 2010-03-03 15:27:29 PST
Bug 34726 shows how to do a this right.
Comment 3 Dumitru Daniliuc 2010-04-07 17:37:36 PDT
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}.
Comment 4 WebKit Review Bot 2010-04-07 17:41:25 PDT
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.
Comment 5 WebKit Review Bot 2010-04-07 18:11:19 PDT
Attachment 52810 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1694006
Comment 6 Dumitru Daniliuc 2010-04-08 16:24:05 PDT
Created attachment 52913 [details]
patch
Comment 7 Eric Seidel (no email) 2010-04-08 16:33:58 PDT
Attachment 52913 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1609345
Comment 8 WebKit Review Bot 2010-04-08 19:15:05 PDT
Attachment 52913 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1671181
Comment 9 Adam Barth 2010-04-08 22:19:03 PDT
So much copy/paste code.  :(

+ RefPtr<Frame> m_frame

I though we decided we wanted to hold a ScriptExecutionContext instead of a m_frame.
Comment 10 Jeremy Orlow 2010-04-09 05:36:09 PDT
(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.
Comment 11 Geoffrey Garen 2010-04-09 10:04:11 PDT
JSC looks good to me.
Comment 12 Eric Seidel (no email) 2010-04-09 14:10:11 PDT
Comment on attachment 52913 [details]
patch

Looks like this breaks the mac and gtk builders.
Comment 13 Dumitru Daniliuc 2010-04-09 19:35:47 PDT
Created attachment 53030 [details]
patch

Same patch. Changed some #includes to make them similar in all callbacks. Should build on all ports now.
Comment 14 Adam Barth 2010-04-12 23:01:07 PDT
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.
Comment 15 Dumitru Daniliuc 2010-04-13 15:16:03 PDT
Landed as r57530.