Bug 40112 – Database callbacks are made using the ScriptExecutionContext of the frame that owns the Database object, rather than that of the caller

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

Database callbacks are made using the ScriptExecutionContext of the frame that owns the Database object. Instead, callbacks should be made using the ScriptContext of the caller that supplied the callback function.

Also, it looks like the Database object holds a RefPtr is to it's owning frame's context, but I don't think that the Database object should prolong the lifetime of the ScriptExecution context. See https://bugs.webkit.org/show_bug.cgi?id=39388#c6 Instead, if the relevant context no longer exists, the callbacks should simply not be made.

I tried writing a LayoutTest to demonstrate this, but had difficulty as the Database object already prevents callbacks once the frame owning the ScriptExecutionContext has gone away, via its stop() method.

See Bug 39879 for related discussion

[Jeremy Orlow]

Note that Andrei is currently working on a patch that'll make [CallWith=ScriptExecutionContext] work with normal function calls.  This should make it easy for non-custom code to get the ScriptExecutionContext so the callbacks/events/etc they create can be ActiveDOMObjects...which is what they should be.  (For example, they should also not fire when execution is currently suspended.  See IDBRequest for an example of an ActiveDOMObject which already handles this gracefully.)

[Dumitru Daniliuc]

Created attachment 58754 [details]
patch

Andrei, Jeremy: please take a look at the patch and let me know if I completely misunderstood what this bug is about.

Adam: assuming I understood correctly what the problem is, can you please take a careful look at this patch? In particular, I'm concerned about ASSERT(m_scriptExecutionContext) in handleEvent() in the JSC callbacks. It seems to me that it's possible for the context that created the callback to go away before we get a chance to call the callback. In that case, should the ASSERT be replaced with something like "if (!m_scriptExecutionContext) return true;"? Or just remove the ASSERT and move the check inside toJSDOMGlobalObject()? Same question for ASSERT(m_data).

[Dumitru Daniliuc]

Btw, I don't have permission to see bug 39388, so if there's anything relevant in there that I should take a look at, you'll probably have to copy-paste that stuff in here (assuming that's allowed!).

[WebKit Review Bot]

Attachment 58754 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3337151

[Dumitru Daniliuc]

(In reply to comment #1)
> so the callbacks/events/etc they create can be ActiveDOMObjects

This might be tough to do. For example, the DB callbacks can be destroyed on the DB thread. However, ~ActiveDOMObject() doesn't allow that, and refactoring the code so that all DB callbacks are destroyed on the main/context thread is quite tricky and in most cases leads to hacks, or at least code that's hard to follow (I tried).

[Dumitru Daniliuc]

Created attachment 58764 [details]
patch

Same patch, should compile on Chromium hopefully.

[WebKit Review Bot]

Attachment 58764 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3306150

[Jeremy Orlow]

(In reply to comment #5)
> (In reply to comment #1)
> > so the callbacks/events/etc they create can be ActiveDOMObjects
> 
> This might be tough to do. For example, the DB callbacks can be destroyed on the DB thread. However, ~ActiveDOMObject() doesn't allow that, and refactoring the code so that all DB callbacks are destroyed on the main/context thread is quite tricky and in most cases leads to hacks, or at least code that's hard to follow (I tried).

I haven't looked at the code yet.  But DOMTimer is apparently another model that you might want to look at.

And/or you could split things into 2 objects: one that hangs out (mostly) on the main thread and one that hangs out (mostly) on the database thread.  One would own the other and make sure the other gets destroyed on the proper thread.  At least conceptually, such a split would probably make things a bit cleaner anyway.

...anyway, will look at the patch today and get back to you either way.

[Dumitru Daniliuc]

Created attachment 58766 [details]
patch

Same patch, this time tested in Chromium.

[Adam Barth]

Comment on attachment 58766 [details]
patch

Great!  Is there a way to test this change?

WebCore/bindings/js/JSCustomSQLStatementErrorCallback.cpp: 
 +      args.append(toJS(exec, deprecatedGlobalObjectForPrototype(exec), error));
How did we get rid of deprecatedGlobalObjectForPrototype(exec) here?  can we get the right global object?

WebCore/bindings/scripts/CodeGeneratorV8.pm:2291
 +              @args = ();
We don't need a "my" here?

[Dumitru Daniliuc]

(In reply to comment #10)
> (From update of attachment 58766 [details])
> Great!  Is there a way to test this change?

Maybe... How do I create 2 ScriptExecutionContexts, such that context_2 can see everything in context_1?

> WebCore/bindings/js/JSCustomSQLStatementErrorCallback.cpp: 
>  +      args.append(toJS(exec, deprecatedGlobalObjectForPrototype(exec), error));
> How did we get rid of deprecatedGlobalObjectForPrototype(exec) here?  can we get the right global object?

That's the global object we get in the generated callbacks. Also, JSDOMBinding.h says:
  // FIXME: Callers to this function should be using the global object
  // from which the object is being created, instead of assuming the lexical one.
  // e.g. subframe.document.body should use the subframe's global object, not the lexical one.

And other than JSDOMBinding.{h|cpp}, there are only 4 calls to this function, all in custom callbacks.


> WebCore/bindings/scripts/CodeGeneratorV8.pm:2291
>  +              @args = ();
> We don't need a "my" here?

It's declared before that with a "my", I just want to clear it here and reuse it. If I add "my" again, wouldn't that count as re-declaring the same variable? Is there a better way to clear the array?

[Adam Barth]

ok

[Adam Barth]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 58766 [details] [details])
> > Great!  Is there a way to test this change?
> 
> Maybe... How do I create 2 ScriptExecutionContexts, such that context_2 can see everything in context_1?

Just use two frames in the same origin.

[Jeremy Orlow]

I took a look and this seems like a good start, but I believe it still doesn't handle the case that you start something that has a callback and then you navigate the page away.  I believe the callback will still fire...possibly on an invalid script execution context.  Is there anything that would prevent this?

Also, if execution is paused because of the inspector's debugger has paused things, it's in the page cache, or other cases like this, I believe the callback will still fire erroneously.  Using ActiveDOMObject would also give you information about this happening.

Once again, IDBRequest and/or DOMTimer should be good things to look at to see how others handle this stuff.

[Dumitru Daniliuc]

Created attachment 59238 [details]
patch

It looks like all DB callbacks are currently called in the correct context; the test I added in this patch passes in an unmodified client. I'm not 100% sure how this happens (I would've thought that they get executed in the context in which the DB handle is created), but I guess keeping track of the "world" in which the callbacks are created takes care of this problem.

In any case, I think it's worth turning callbacks into ActiveDOMObjects.

It might be hard to figure out what changes have been made to WebCoreCommon.vsprops: I only added WebCore/bindings/generic/ to the list of include dirs.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/generic/ActiveDOMCallback.h:56:  Use 0 instead of NULL.  [readability/null] [4]
WebCore/bindings/generic/ActiveDOMCallback.cpp:94:  Use 0 instead of NULL.  [readability/null] [4]
WebCore/bindings/generic/ActiveDOMCallback.cpp:96:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 3 in 42 files


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

[Dumitru Daniliuc]

> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/bindings/generic/ActiveDOMCallback.h:56:  Use 0 instead of NULL.  [readability/null] [4]
> WebCore/bindings/generic/ActiveDOMCallback.cpp:94:  Use 0 instead of NULL.  [readability/null] [4]
> WebCore/bindings/generic/ActiveDOMCallback.cpp:96:  Use 0 instead of NULL.  [readability/null] [4]
> Total errors found: 3 in 42 files

All 3 NULLs are in comments. check-webkit-style should probably be able to detect this.

[WebKit Review Bot]

Attachment 59238 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3291533

[Jeremy Orlow]

Comment on attachment 59238 [details]
patch

WebCore/bindings/generic/ActiveDOMCallback.cpp:14
 +   *     * Neither the name of Google Inc. nor the names of its
I believe we prefer new files to be the 2 clause BSD license found here: http://webkit.org/coding/bsd-license.html  (Changing Apple to Google is not necessary except in the copyright at the top, as far as I understand it.)

WebCore/bindings/generic/ActiveDOMCallback.h:14
 +   *     * Neither the name of Google Inc. nor the names of its
Ditto

WebCore/bindings/generic/ActiveDOMCallback.h:58
 +      WTF::Mutex* m_implMutex;
This should be an OwnPtr<Mutex>

WebCore/bindings/generic/ActiveDOMCallback.h:63
 +      ActiveDOMObjectCallbackImpl* m_impl;
Ditto.  You can .release() them when you pass them to the destroyOnContextThread function.

WebCore/bindings/generic/ActiveDOMCallback.cpp:40
 +  static void destroyOnContextThread(ActiveDOMObjectCallbackImpl*, Mutex*, bool);
Would it be better to put all of this stuff in an anonymous name space?  (I can't remember if this is a standard thing to do in WebKit or not.)

LayoutTests/storage/resources/database-common.js:36
 +  function setupAndRunFramesTest()
This function seems pretty specific to the way that you've set up this particular test.  I think it should probably just go in the test itself.

WebCore/bindings/js/JSCustomSQLStatementErrorCallback.cpp:46
 +      if (!m_data || !canInvokeCallback())
Shouldn't we queue this up to run whenever we can invoke the callback?

LayoutTests/storage/resources/callbacks-are-called-in-correct-context-test-frame.html:15
 +      db.transaction(top.frames[2].failingTransactionCallback, top.frames[2].errorTransactionCallback);
God...this is all so confusing....but isn't the callback supposed to bound to _this_ scriptExecutioncontext (and not the other frame's) and thus we expect the above logCallback to be called rather than the one in the other file?

Dr Barth, thoughts?

WebCore/bindings/generic/ActiveDOMCallback.h:45
 +  class ActiveDOMCallback {
Should this be threadsafe refcounted?  Or will there always be clear ownership?

WebCore/bindings/generic/ActiveDOMCallback.cpp:91
 +  static void destroyOnContextThread(ActiveDOMObjectCallbackImpl* impl, Mutex* implMutex, bool deleteMutex)
This should take passOwnPtrs...that makes it easier to make sure no case leaks.

WebCore/bindings/generic/ActiveDOMCallback.cpp:94
 +          // implMutex is NULL if and only if ActiveDOMCallback's destructor was already called and impl
But are we _guaranteed_ that tasks scheduled on a thread are run in order?  If not, then it's possible the destruction task (which deletes the mutex) could run first.  I think in practice they run in order, but I'm not sure we have a guarantee of it.

WebCore/bindings/generic/ActiveDOMCallback.cpp:78
 +      virtual void contextDestroyed() { destroyOnContextThread(this, m_mutex, false); }
I believe this will _always_ be called before the destructor.  Using this fact might help you clean up the logic a bit.

One idea: the mutex is owned by this class (and just destroys it normally).  The destroyOnContextThread task holds a reference to this object (which is made threadsafe ref counted).  It releases its reference once it's deleted the impl on the context thread.  That way the mutex can't go away before the task uses it but you don't need the deleteMutex flag and you don't have the second task for every instance.

[Dumitru Daniliuc]

Created attachment 59355 [details]
patch

> WebCore/bindings/generic/ActiveDOMCallback.cpp:14
>  +   *     * Neither the name of Google Inc. nor the names of its
> I believe we prefer new files to be the 2 clause BSD license found here: http://webkit.org/coding/bsd-license.html  (Changing Apple to Google is not necessary except in the copyright at the top, as far as I understand it.)

All classes in WebCore/bindings/generic/ have this copyright. There's another change too: in the last paragraph, "APPLE" is changed to "THE COPYRIGHT HOLDERS". I vaguely remember a discussion about this and I think the verdict was that it's OK to use this copyright for new code introduced by Google, but I might be wrong.

> WebCore/bindings/generic/ActiveDOMCallback.h:58
>  +      WTF::Mutex* m_implMutex;
> This should be an OwnPtr<Mutex>

Made some changes, the code should look a lot simpler now.

> WebCore/bindings/generic/ActiveDOMCallback.cpp:40
>  +  static void destroyOnContextThread(ActiveDOMObjectCallbackImpl*, Mutex*, bool);
> Would it be better to put all of this stuff in an anonymous name space?  (I can't remember if this is a standard thing to do in WebKit or not.)

I don't see the guide saying anything about static functions. Normally, I'd prefer an anonymous namespace too, but in this case it's only one simple function, so I don't think it's worth it (plus I'd have to prefix all classes with WebCore:: too).

> LayoutTests/storage/resources/database-common.js:36
>  +  function setupAndRunFramesTest()
> This function seems pretty specific to the way that you've set up this particular test.  I think it should probably just go in the test itself.

Done. The setup itself could probably be reused, but I don't know if we're gonna have any more DB tests that use frames.

> WebCore/bindings/js/JSCustomSQLStatementErrorCallback.cpp:46
>  +      if (!m_data || !canInvokeCallback())
> Shouldn't we queue this up to run whenever we can invoke the callback?

You mean invoke the callback one more time when we can? What if the code that needs to run next depends on the result of this callback? For example, we cannot move on to the next sync transaction until the success/error callback of the current one returns. Unless ScriptExecutionContext::suspendActiveDOMObjects() suspends the context thread too, it seems to me that there's not much we can do.

> LayoutTests/storage/resources/callbacks-are-called-in-correct-context-test-frame.html:15
>  +      db.transaction(top.frames[2].failingTransactionCallback, top.frames[2].errorTransactionCallback);
> God...this is all so confusing....but isn't the callback supposed to bound to _this_ scriptExecutioncontext (and not the other frame's) and thus we expect the above logCallback to be called rather than the one in the other file?
> 
> Dr Barth, thoughts?

Hmm, not sure about the test. It looks like the callbacks are executed in the context where they're declared. I had the same expectations as you, but both the JS and V8 bindings behave this way (and they get the context in different ways), and I can't find anything obviously wrong with them... Adam, can you please take a look at the test and clarify what the expectations are?

> WebCore/bindings/generic/ActiveDOMCallback.h:45
>  +  class ActiveDOMCallback {
> Should this be threadsafe refcounted?  Or will there always be clear ownership?

I don't think ActiveDOMCallback should be (threadsafe) refcounted, because it is extended by all callback implementations, and some callbacks just don't need to be refcounted. Instead, every callback implementation extends an abstract class used by the non-bindings code to refer to that callback; I think the refcounted flavor (if any) should go into the declaration of that abstract class.

> WebCore/bindings/generic/ActiveDOMCallback.cpp:94
>  +          // implMutex is NULL if and only if ActiveDOMCallback's destructor was already called and impl
> But are we _guaranteed_ that tasks scheduled on a thread are run in order?  If not, then it's possible the destruction task (which deletes the mutex) could run first.  I think in practice they run in order, but I'm not sure we have a guarantee of it.

This shouldn't be an issue anymore.

> WebCore/bindings/generic/ActiveDOMCallback.cpp:78
>  +      virtual void contextDestroyed() { destroyOnContextThread(this, m_mutex, false); }
> I believe this will _always_ be called before the destructor.  Using this fact might help you clean up the logic a bit.

Technically, the destructor of a callback can be called on a different thread before contextDestroyed() is called (on the context thread). However, I don't see how contextDestroyed() can be called after destroyOnContextThread() is called by the callback destructor and executed on the context thread. That helped simply the implementation a lot. :)

[WebKit Review Bot]

Attachment 59355 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3292609

[Dumitru Daniliuc]

(In reply to comment #21)
> Attachment 59355 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/3292609

I'm told that at least one step in the build process is skipped on the Chromium bots. The patch build just fine in my local Chromium client.

[Jeremy Orlow]

(In reply to comment #20)
> Created an attachment (id=59355) [details]
> patch
> 
> > WebCore/bindings/generic/ActiveDOMCallback.cpp:14
> >  +   *     * Neither the name of Google Inc. nor the names of its
> > I believe we prefer new files to be the 2 clause BSD license found here: http://webkit.org/coding/bsd-license.html  (Changing Apple to Google is not necessary except in the copyright at the top, as far as I understand it.)
> 
> All classes in WebCore/bindings/generic/ have this copyright. There's another change too: in the last paragraph, "APPLE" is changed to "THE COPYRIGHT HOLDERS". I vaguely remember a discussion about this and I think the verdict was that it's OK to use this copyright for new code introduced by Google, but I might be wrong.

It probably doesn't matter much either way.

> > LayoutTests/storage/resources/database-common.js:36
> >  +  function setupAndRunFramesTest()
> > This function seems pretty specific to the way that you've set up this particular test.  I think it should probably just go in the test itself.
> 
> Done. The setup itself could probably be reused, but I don't know if we're gonna have any more DB tests that use frames.

Well, it was pretty specific to that way of setting up frames and was only a couple lines.

> > WebCore/bindings/js/JSCustomSQLStatementErrorCallback.cpp:46
> >  +      if (!m_data || !canInvokeCallback())
> > Shouldn't we queue this up to run whenever we can invoke the callback?
> 
> You mean invoke the callback one more time when we can? What if the code that needs to run next depends on the result of this callback? For example, we cannot move on to the next sync transaction until the success/error callback of the current one returns. Unless ScriptExecutionContext::suspendActiveDOMObjects() suspends the context thread too, it seems to me that there's not much we can do.

Well, it depends on why it's suspended.  If it's because we're debugging, then we probably shouldn't do anything special.  If it's because it's in the page cache, you're right.  Is there any way to distinguish between the two?

Also, an onsuccess call when the page is in the page cache should probably cancel the transaction and result in an error call being made instead.  But an onerror call should be queued up no matter what.
 
> > LayoutTests/storage/resources/callbacks-are-called-in-correct-context-test-frame.html:15
> >  +      db.transaction(top.frames[2].failingTransactionCallback, top.frames[2].errorTransactionCallback);
> > God...this is all so confusing....but isn't the callback supposed to bound to _this_ scriptExecutioncontext (and not the other frame's) and thus we expect the above logCallback to be called rather than the one in the other file?
> > 
> > Dr Barth, thoughts?
> 
> Hmm, not sure about the test. It looks like the callbacks are executed in the context where they're declared. I had the same expectations as you, but both the JS and V8 bindings behave this way (and they get the context in different ways), and I can't find anything obviously wrong with them... Adam, can you please take a look at the test and clarify what the expectations are?

We just talked about this a bunch and Steve Block thinks your right, but we're not 100% sure.  We really need an expert on this to explain what the expected behavior is.  i.e. what do we do in the following situation: 3 frames in the same origin named A, B, C.  Frame B has function X in it.  Frame A calls frame C's windows' openDatabase method and passes in function X from frame B.

If the answer is B then I guess I'll need to change the indexedDB code.
 
> > WebCore/bindings/generic/ActiveDOMCallback.cpp:78
> >  +      virtual void contextDestroyed() { destroyOnContextThread(this, m_mutex, false); }
> > I believe this will _always_ be called before the destructor.  Using this fact might help you clean up the logic a bit.
> 
> Technically, the destructor of a callback can be called on a different thread before contextDestroyed() is called (on the context thread). However, I don't see how contextDestroyed() can be called after destroyOnContextThread() is called by the callback destructor and executed on the context thread. That helped simply the implementation a lot. :)

For the record, I still think it's a mistake (because it's messy) to ever allow callback objects to be destroyed on a background thread.  I don't know this code very well, but it seems like you could have wrapped the callback object the bindings gives you in a threadsafe object that knows how to destroy the callback safely.

[Alexey Proskuryakov]

> Normally, I'd prefer an anonymous namespace too

We don't normally use anonymous namespaces in WebKit. An explanation I saw was that it's hard to refer to symbols defined in such in a debugger.

[Eric Seidel (no email)]

Comment on attachment 58766 [details]
patch

Cleared Adam Barth's review+ from obsolete attachment 58766 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Jeremy Orlow]

(In reply to comment #23)

Ping...  (Was hoping for a response to my comments.)

[Dumitru Daniliuc]

> > You mean invoke the callback one more time when we can? What if the code that needs to run next depends on the result of this callback? For example, we cannot move on to the next sync transaction until the success/error callback of the current one returns. Unless ScriptExecutionContext::suspendActiveDOMObjects() suspends the context thread too, it seems to me that there's not much we can do.
> 
> Well, it depends on why it's suspended.  If it's because we're debugging, then we probably shouldn't do anything special.  If it's because it's in the page cache, you're right.  Is there any way to distinguish between the two?

I see your point about debugging, and I agree that in that case we should go ahead and execute the callback (is that what you were suggesting?). However, I'm not sure how to figure out if we're in debug mode or not, I'll ask around.

> Also, an onsuccess call when the page is in the page cache should probably cancel the transaction and result in an error call being made instead.  But an onerror call should be queued up no matter what.

This is not possible. According to the spec, the success callback is supposed to be called only after the transaction has been successfully committed. At that point there's no way to revert it.

Also, we cannot just queue up an error callback. The return value of a statement error callback determines if we should execute the other statements in the transaction, or if we should immediately fail the transaction. So in a sync transaction, we cannot do anything until that error callback has executed, which means that we need to either block until the context is resumed and we can execute the callback, or we need to fail the callback (basically, assume that it threw an exception), and move on.

> We just talked about this a bunch and Steve Block thinks your right, but we're not 100% sure.  We really need an expert on this to explain what the expected behavior is.  i.e. what do we do in the following situation: 3 frames in the same origin named A, B, C.  Frame B has function X in it.  Frame A calls frame C's windows' openDatabase method and passes in function X from frame B.

I'll try to catch Adam online tonight and bug him about this.

> For the record, I still think it's a mistake (because it's messy) to ever allow callback objects to be destroyed on a background thread.  I don't know this code very well, but it seems like you could have wrapped the callback object the bindings gives you in a threadsafe object that knows how to destroy the callback safely.

All async callbacks _are_ ThreadSafeShared (take a look at SQLTransactionCallback.h, DatabaseCallback.h, and so on). Other callbacks might not need to be ThreadSafeShared, though. For example, the entire sync DB implementation is going to live only on the context thread, so the JSC and V8 implementations of SQLTransactionSyncCallback.h don't need to be ThreadSafeShared.

So I think we shouldn't make ActiveDOMCallback ThreadSafeShared, and thus force all callback objects to be ThreadSafeShared. Instead, we should leave it up to "interface" declarations like SQLTransactionCallback and SQLTransactionSyncCallback to decide if the implementations of those callback objects need to be ThreadSafeShared or not.

[Dumitru Daniliuc]

Created attachment 59751 [details]
patch

I talked to Adam about what should run where, and I updated the test expectations accordingly (assuming I understood him correctly). However, I can't get the test to pass. So I'm uploading the patch to have something to point Adam to, and hopefully he can tell me what I'm doing wrong. :)

I believe Adam's answer was that the callback objects should be created in the context in which the callbacks are invoked. However, the functions/variables/etc. that the callbacks "see" while they run should be the ones in the context in which they were declared. So basically, if you have frames A, B, and C, and each one of them has a log() method, and the DB handle is opened in A, and the transaction callbacks are declared in B (and they call log()), then when you run db.transaction(callback) in frame C, the callback objects should be created in frame C, but the log() method in frame B should be called.

When I run the test, it does always call log() in the frame in which the callbacks are declared. However, it seems like the callback objects are always created in that frame too -- that's the part that's failing.

[Jeremy Orlow]

AP: Are you watching this?  Would love, at very least, for you to sanity check our thinking on this...

(In reply to comment #27)
> > > You mean invoke the callback one more time when we can? What if the code that needs to run next depends on the result of this callback? For example, we cannot move on to the next sync transaction until the success/error callback of the current one returns. Unless ScriptExecutionContext::suspendActiveDOMObjects() suspends the context thread too, it seems to me that there's not much we can do.
> > 
> > Well, it depends on why it's suspended.  If it's because we're debugging, then we probably shouldn't do anything special.  If it's because it's in the page cache, you're right.  Is there any way to distinguish between the two?
> 
> I see your point about debugging, and I agree that in that case we should go ahead and execute the callback (is that what you were suggesting?). However, I'm not sure how to figure out if we're in debug mode or not, I'll ask around.
> 
> > Also, an onsuccess call when the page is in the page cache should probably cancel the transaction and result in an error call being made instead.  But an onerror call should be queued up no matter what.
> 
> This is not possible. According to the spec, the success callback is supposed to be called only after the transaction has been successfully committed. At that point there's no way to revert it.
> 
> Also, we cannot just queue up an error callback. The return value of a statement error callback determines if we should execute the other statements in the transaction, or if we should immediately fail the transaction. So in a sync transaction, we cannot do anything until that error callback has executed, which means that we need to either block until the context is resumed and we can execute the callback, or we need to fail the callback (basically, assume that it threw an exception), and move on.

Then I guess pages that use databases will just never be able to go into the page cache.  (You should probably add a comment explaining all of this in wherever we keep it from happening so people know what they're up against if they want to remove the block.)  In that case, the problem of seeing if the reason your suspended is because of being in the page cache or something like debugging seems to be solved for you.  :-)
 
> > We just talked about this a bunch and Steve Block thinks your right, but we're not 100% sure.  We really need an expert on this to explain what the expected behavior is.  i.e. what do we do in the following situation: 3 frames in the same origin named A, B, C.  Frame B has function X in it.  Frame A calls frame C's windows' openDatabase method and passes in function X from frame B.
> 
> I'll try to catch Adam online tonight and bug him about this.
> 
> > For the record, I still think it's a mistake (because it's messy) to ever allow callback objects to be destroyed on a background thread.  I don't know this code very well, but it seems like you could have wrapped the callback object the bindings gives you in a threadsafe object that knows how to destroy the callback safely.
> 
> All async callbacks _are_ ThreadSafeShared (take a look at SQLTransactionCallback.h, DatabaseCallback.h, and so on). Other callbacks might not need to be ThreadSafeShared, though. For example, the entire sync DB implementation is going to live only on the context thread, so the JSC and V8 implementations of SQLTransactionSyncCallback.h don't need to be ThreadSafeShared.
> 
> So I think we shouldn't make ActiveDOMCallback ThreadSafeShared, and thus force all callback objects to be ThreadSafeShared. Instead, we should leave it up to "interface" declarations like SQLTransactionCallback and SQLTransactionSyncCallback to decide if the implementations of those callback objects need to be ThreadSafeShared or not.

That's not what I was saying.  I think it's bad that _any_ callback needs to be threadsafe.  I think it should be _owned_ by a threadsafe object that ensures it's deleted on the right thread.  But this thought wasn't directed at this patch, it was more general.

[Jeremy Orlow]

(In reply to comment #28)
> Created an attachment (id=59751) [details]
> patch
> 
> I talked to Adam about what should run where, and I updated the test expectations accordingly (assuming I understood him correctly). However, I can't get the test to pass. So I'm uploading the patch to have something to point Adam to, and hopefully he can tell me what I'm doing wrong. :)
> 
> I believe Adam's answer was that the callback objects should be created in the context in which the callbacks are invoked. However, the functions/variables/etc. that the callbacks "see" while they run should be the ones in the context in which they were declared. So basically, if you have frames A, B, and C, and each one of them has a log() method, and the DB handle is opened in A, and the transaction callbacks are declared in B (and they call log()), then when you run db.transaction(callback) in frame C, the callback objects should be created in frame C, but the log() method in frame B should be called.
> 
> When I run the test, it does always call log() in the frame in which the callbacks are declared. However, it seems like the callback objects are always created in that frame too -- that's the part that's failing.

That's insane....guess we have some work to do in these other APIs as well.  :-)

[WebKit Review Bot]

Attachment 59751 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3267700

[Steve Block]

> I believe Adam's answer was that the callback objects should be created in the
> context in which the callbacks are invoked.
You mean the context in which the DB method was called? If I understand your latest patch, that's what you're doing here. And you check that the context is still around before you make the callback, right? I think there should be a test to check that we don't crash in this case.

This is what Geolocation does and it has a test for attempting to call back to a deleted context that you might want to take a look at.

> When I run the test, it does always call log() in the frame in which the
> callbacks are declared. However, it seems like the callback objects are always
> created in that frame too -- that's the part that's failing.
How are you attempting to test (from JS) the context in which the callback is created? When I was making similar changes to Geolocation I didn't find a way to do this. As soon as the callback function is invoked, the context is switched to that of the callback function, as it should be. So in your example, by the time the callback function is invoked, the global object is that of frame B.

On a related note, why do you need to use a CGI script and test the referrer to determine the context. Isn't the referrer simply determined by the global object? So I think we can test any property of the global object, like window.location.

[Alexey Proskuryakov]

> Then I guess pages that use databases will just never be able to go into the page cache.

That's how it works now, but I think that pages should be able to go to page cache when there are no outstanding transactions.

A general comment: it's hard for me to think of a callback as ActiveDOMObject. It's not much of an object! A transaction object might be a better candidate for housekeeping like that.

[Jeremy Orlow]

(In reply to comment #33)
> > Then I guess pages that use databases will just never be able to go into the page cache.
> 
> That's how it works now, but I think that pages should be able to go to page cache when there are no outstanding transactions.
> 
> A general comment: it's hard for me to think of a callback as ActiveDOMObject. It's not much of an object! A transaction object might be a better candidate for housekeeping like that.

We could always rename ActiveDOMObject as well.  :-)

[Alexey Proskuryakov]

> I'm not sure how to figure out if we're in debug mode or not, I'll ask around.

The fact that ActiveDOMObjects are suspended in identical manner for debugging, for page cache, and for displaying a modal dialog is an oversimplification. We've been able to get away with it for now, but it's quite possible that we need different behaviors in those cases. I don't have a specific suggestion here.

[Adam Barth]

Comment on attachment 59751 [details]
patch

Dumi, does this patch reflect your current understanding?  If so, I'm happy to look at it in detail.

[Dumitru Daniliuc]

(In reply to comment #36)
> (From update of attachment 59751 [details])
> Dumi, does this patch reflect your current understanding?  If so, I'm happy to look at it in detail.

Yes, please do when you have some time.

[Adam Barth]

Comment on attachment 59751 [details]
patch

WebCore/bindings/generic/ActiveDOMCallback.cpp:47
 +          return new DestroyOnContextThreadTask(impl);
I think we're supposed to say adoptPtr nowadays.

WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:55
 +      if (!m_data || !m_data->globalObject() || !canInvokeCallback())
I don't understand how !m_data or !m_data->globalObject() can be true here.

[Adam Barth]

Comment on attachment 59751 [details]
patch

This patch is definitely an improvement.  It might take a little tweaking to get perfect, but that's fine.  Thanks for sticking with it.  Please think about my comments before landing.

WebCore/bindings/v8/custom/V8CustomSQLStatementErrorCallback.cpp: 
 +      RefPtr<ScriptExecutionContext> protector(context);
Why don't we need this anymore?

[Dumitru Daniliuc]

to answer the question you asked on IRC: when i run the test, i always get "Referrer: http://127.0.0.1:8000/storage/resources/callbacks-are-called-in-correct-context-second-frame.html", no matter where in which frame i run db.transaction().

(In reply to comment #38)
> (From update of attachment 59751 [details])
> WebCore/bindings/generic/ActiveDOMCallback.cpp:47
>  +          return new DestroyOnContextThreadTask(impl);
> I think we're supposed to say adoptPtr nowadays.

changed.

> WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:55
>  +      if (!m_data || !m_data->globalObject() || !canInvokeCallback())
> I don't understand how !m_data or !m_data->globalObject() can be true here.

removed. i don't think they can, but i wasn't sure. if globalObject is null though (is it even possible?), then we'll crash in the constructor.

(In reply to comment #39)
> (From update of attachment 59751 [details])
> This patch is definitely an improvement.  It might take a little tweaking to get perfect, but that's fine.  Thanks for sticking with it.  Please think about my comments before landing.
> 
> WebCore/bindings/v8/custom/V8CustomSQLStatementErrorCallback.cpp: 
>  +      RefPtr<ScriptExecutionContext> protector(context);
> Why don't we need this anymore?

the 'context' argument is no longer passed to the callback, we get it from ActiveDOMCallback. also, the callback is always called on the context thread, and ScriptExecutionContext isn't thread-safe, so i'm assuming it can only be destroyed on the context thread. therefore, there can't be a race between handleEvent() (that uses ScriptExecutionContext*) and ActiveDOMObject::contextDestroyed() (that sets it to NULL).

[Dumitru Daniliuc]

Created attachment 63135 [details]
patch

Same patch, with all conflicts resolved, and the expectations for the new test updated.

[Jeremy Orlow]

You already have an r+.  If you didn't change anything of substance, you probably can just land as is.

[Adam Barth]

Comment on attachment 63135 [details]
patch

(Yeah, I said the same thing to dumi on IRC.)

[Dumitru Daniliuc]

landed: r64537.

[Alexey Proskuryakov]

+// Instances of this class are accessed only on the context thread, so we don't need to use locks.

I don't understand this comment in ActiveDOMCallback.cpp. Most functions in this class lock m_mutex.

[Dumitru Daniliuc]

(In reply to comment #45)
> +// Instances of this class are accessed only on the context thread, so we don't need to use locks.
> 
> I don't understand this comment in ActiveDOMCallback.cpp. Most functions in this class lock m_mutex.

This comment is obsolete. In the first version of this patch, ActiveDOMObjectCallbackImpl instances were used only on the context thread, and I forgot to delete the comment when that changed. I'll upload a patch to remove it.

[Dumitru Daniliuc]

Created attachment 67136 [details]
remove obsolete comment

[Dumitru Daniliuc]

the patch that removes the comment landed in r67153.

[Yong Li]

95 bool canInvokeCallback()
 96 {
 97 MutexLocker locker(m_mutex);
 98 return (!m_suspended && !m_stopped);
 99 } 

So when the ActiveDOMObject is suspended, the callback is not allowed to run. How do we resume the callback when ActiveDOMObject is resumed?

[Dumitru Daniliuc]

(In reply to comment #49)
> 95 bool canInvokeCallback()
>  96 {
>  97 MutexLocker locker(m_mutex);
>  98 return (!m_suspended && !m_stopped);
>  99 } 
> 
> So when the ActiveDOMObject is suspended, the callback is not allowed to run. How do we resume the callback when ActiveDOMObject is resumed?

We don't (at the moment). To preserve the order in which things should run, we need to add a special "higher priority" task queue to the main thread, and post tasks to re-invoke the callbacks there. I'm looking into that now.

[Jeremy Orlow]

(In reply to comment #50)
> (In reply to comment #49)
> > 95 bool canInvokeCallback()
> >  96 {
> >  97 MutexLocker locker(m_mutex);
> >  98 return (!m_suspended && !m_stopped);
> >  99 } 
> > 
> > So when the ActiveDOMObject is suspended, the callback is not allowed to run. How do we resume the callback when ActiveDOMObject is resumed?
> 
> We don't (at the moment). To preserve the order in which things should run, we need to add a special "higher priority" task queue to the main thread, and post tasks to re-invoke the callbacks there. I'm looking into that now.

Dear god, no.  Priorities are almost never the right answer when there needs to be a strict ordering of things.

Just keep your own queue somewhere and have the task run through items in the queue.

[Yong Li]

(In reply to comment #51)
> (In reply to comment #50)
> > (In reply to comment #49)
> > > 95 bool canInvokeCallback()
> > >  96 {
> > >  97 MutexLocker locker(m_mutex);
> > >  98 return (!m_suspended && !m_stopped);
> > >  99 } 
> > > 
> > > So when the ActiveDOMObject is suspended, the callback is not allowed to run. How do we resume the callback when ActiveDOMObject is resumed?
> > 
> > We don't (at the moment). To preserve the order in which things should run, we need to add a special "higher priority" task queue to the main thread, and post tasks to re-invoke the callbacks there. I'm looking into that now.
> Dear god, no.  Priorities are almost never the right answer when there needs to be a strict ordering of things.
> Just keep your own queue somewhere and have the task run through items in the queue.

I'm working on a patch to use a task queue in Document class. When page loading is deferred, I push the tasks into queue. Will create a bug for it.

[Yong Li]

(In reply to comment #52)
> (In reply to comment #51)
> > (In reply to comment #50)
> > > (In reply to comment #49)
> > > > 95 bool canInvokeCallback()
> > > >  96 {
> > > >  97 MutexLocker locker(m_mutex);
> > > >  98 return (!m_suspended && !m_stopped);
> > > >  99 } 
> > > > 
> > > > So when the ActiveDOMObject is suspended, the callback is not allowed to run. How do we resume the callback when ActiveDOMObject is resumed?
> > > 
> > > We don't (at the moment). To preserve the order in which things should run, we need to add a special "higher priority" task queue to the main thread, and post tasks to re-invoke the callbacks there. I'm looking into that now.
> > Dear god, no.  Priorities are almost never the right answer when there needs to be a strict ordering of things.
> > Just keep your own queue somewhere and have the task run through items in the queue.
> I'm working on a patch to use a task queue in Document class. When page loading is deferred, I push the tasks into queue. Will create a bug for it.

Bug 49401 fired for this

[Yong Li]

When a database transaction succeeds, JSCustomVoidCallback is called. Shouldn't we also make it ActiveDOMCallback?

Bug 73945 opened for this.