Bug 40112

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.)

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).

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!).

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

(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).

Created attachment 58764 [details] patch Same patch, should compile on Chromium hopefully.

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

(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.

Created attachment 58766 [details] patch Same patch, this time tested in Chromium.

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?

(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?

ok

(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.

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.

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.

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.

> 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.

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

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.

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. :)

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

(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.

(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.

> 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.

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.

(In reply to comment #23) Ping... (Was hoping for a response to my comments.)

> > 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.

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.

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.

(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. :-)

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

> 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.

> 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.

(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. :-)

> 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.

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.

(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.

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.

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?

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).

Created attachment 63135 [details] patch Same patch, with all conflicts resolved, and the expectations for the new test updated.

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

Comment on attachment 63135 [details] patch (Yeah, I said the same thing to dumi on IRC.)

landed: r64537.

+// 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.

(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.

Created attachment 67136 [details] remove obsolete comment

the patch that removes the comment landed in r67153.

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?

(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.

(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.

(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.

(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

When a database transaction succeeds, JSCustomVoidCallback is called. Shouldn't we also make it ActiveDOMCallback? Bug 73945 opened for this.