[webkit-reviews] review requested: [Bug 40112] Database callbacks are made using the ScriptExecutionContext of the frame that owns the Database object, rather than that of the caller : [Attachment 59355] patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has asked	for review:
Bug 40112: Database callbacks are made using the ScriptExecutionContext of the
frame that owns the Database object, rather than that of the caller
https://bugs.webkit.org/show_bug.cgi?id=40112

Attachment 59355: patch
https://bugs.webkit.org/attachment.cgi?id=59355&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
> 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-fram
e.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. :)