[webkit-reviews] review denied: [Bug 40112] Database callbacks are made using the ScriptExecutionContext of the frame that owns the Database object, rather than that of the caller : [Attachment 59238] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 21 08:45:08 PDT 2010
Jeremy Orlow <jorlow at chromium.org> has denied Dumitru Daniliuc
<dumi at chromium.org>'s request 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 59238: patch
https://bugs.webkit.org/attachment.cgi?id=59238&action=review
------- Additional Comments from Jeremy Orlow <jorlow 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.)
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-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?
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.
More information about the webkit-reviews
mailing list