Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
780793002:
Make CryptoResultImpl not to use WeakPtr. (Closed)
DescriptionMake CryptoResultImpl not to use WeakPtr.
To move ScriptPromiseResolver on oilpan heap, need to remove WeakPtr.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187118
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 14
Patch Set 4 : #
Total comments: 13
Patch Set 5 : #Messages
Total messages: 31 (6 generated)
Patchset #1 (id:1) has been deleted
tasak@google.com changed reviewers: + haraken@chromium.org, yhirano@chromium.org
Would you review this CL? https://codereview.chromium.org/780793002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/ScriptPromiseResolver.h (right): https://codereview.chromium.org/780793002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptPromiseResolver.h:88: bool stopped() { return m_resolver.cleared(); } Used for ASSERTION in ~CryptoResultImpl. https://codereview.chromium.org/780793002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptPromiseResolver.h:96: virtual void resolverCleared() { } Invoked in ScriptPromiseResolver::clear(). https://codereview.chromium.org/780793002/diff/20001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/20001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:103: ASSERT(!m_resolver || m_resolver->stopped()); Check whether WeakResoler has been already stopped or not. If WeakResolver is still alive here, it might use CryptoResultImpl::cancel(), i.e. heap-use-after-free. https://codereview.chromium.org/780793002/diff/20001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:181: m_resolver = nullptr; To avoid the following ASSERTION failure, CryptoResultImpl needs to reset m_resolver in cancel(): ASSERTION FAILED: m_verifier.isSafeToUse() ../../third_party/WebKit/Source/wtf/RefCounted.h(107) : bool WTF::RefCountedBase::derefBase() 1 0x7fb8fd7ba113 ... 19 0x7fb9082867a5 blink::WebCryptoResult::reset() ... 30 0x7fb907391e14 base::SequencedWorkerPool::Inner::ThreadLoop(base::SequencedWorkerPool::Worker*) 31 0x7fb90739175a base::SequencedWorkerPool::Worker::Run()
My understanding in the previous discussion was that calling cancel() in the destructor is the problem. I don't understand why WeakPtr is problematic: Can you tell me?
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org
On 2014/12/04 13:19:49, yhirano wrote: > My understanding in the previous discussion was that calling cancel() in the > destructor is the problem. I don't understand why WeakPtr is problematic: Can > you tell me? We're moving ScriptPromiseResolver to Oilpan's heap. This means that ScriptPromiseResolver is not destructed until a next GC runs. This means that the timing when cancel() is called depends on the timing of GC. This is not good because GC should not be observable to outside. We need to explicitly cancel the CryptoResult at the exact timing when it should be canceled. In my understanding, the timing is when the Promise is resolved or rejected.
yhirano@chromium.org changed reviewers: + eroman@chromium.org
Thanks. +eroman Eric, I have a question: Is calling CryptoResultImpl::cancel needed in resolving / rejecting cases? If not, we can create a derived function ::stop in WeakResolver and move the call into it.
The problem is that CryptoResultImpl is passed between threads. * ~CryptoResultImpl() can be called on a different thread * CryptoResultImpl may outlive the blink thread Therefore CryptResultImpl cannot hold a RefPtr<ScriptPromiseResolver> as this CL proposes, since ScriptPromiseResolver is *not* RefCountedThreadSafe. Even if it were, I don't think it would be a good idea to extend the lifetime of the object beyond the Blink thread. The other requirement for CryptoResultImpl is that it needs to be able to test for cancellation from a non-blink thread. The way this currently works, when the ScriptPromiseResolver is abandoned (because the ExecutionContext was torn down), it marks the CryptoResultImpl as having been cancelled (which is tested for optimistically on the worker thread).
Thank you for the info. Yeah, I would like to keep the current design, too. I'm wondering if the following works: - Keep the current design. - Add WeakResolver::stop that overrides ScriptPromiseResolver::stop. - Move |m_result->cancel()| from the destructor to |stop|. Note that |m_result->cancel()| is not called when resolved or rejected. My question is, is it a problem?
> Note that |m_result->cancel()| is not called when resolved or rejected. > My question is, is it a problem? No, that is not a problem.
On 2014/12/04 19:32:57, eroman wrote: > The problem is that CryptoResultImpl is passed between threads. > > * ~CryptoResultImpl() can be called on a different thread > * CryptoResultImpl may outlive the blink thread > > Therefore CryptResultImpl cannot hold a RefPtr<ScriptPromiseResolver> as this CL > proposes, since ScriptPromiseResolver is *not* RefCountedThreadSafe. Even if it > were, I don't think it would be a good idea to extend the lifetime of the object > beyond the Blink thread. > > The other requirement for CryptoResultImpl is that it needs to be able to test > for cancellation from a non-blink thread. > > The way this currently works, when the ScriptPromiseResolver is abandoned > (because the ExecutionContext was torn down), it marks the CryptoResultImpl as > having been cancelled (which is tested for optimistically on the worker thread). Would it be possible to post a task so that CryptoResultImpl is accessed only by the main thread?
On 2014/12/04 22:46:51, eroman wrote: > > Note that |m_result->cancel()| is not called when resolved or rejected. > > My question is, is it a problem? > > No, that is not a problem. tasak@, then I propose to fix the problem in that way.
That is in fact how it is implemented: (1) The request starts on a Blink thread. (2) The CryptoResultImpl is passed across the Blink API the the content implementation of WebCryto. (3) A task is posted to the crypto background thread (4) On completion of the encrypt/decrypt/etc a task is posted back to the origin Blink thread. (5) A method is called on CryptoResultImpl to either complete/reject the Promise So in theory, CryptoResultImpl is operated exclusively on the Blink thread that started the request. ... Except for one annoying problem: WebWorker threads. In the case when the originating Blink thread for the request was a WebWorker thread, the Chromium-side of WebCrypto gets a handle to the thread it needs to post back to in step (4) using: WorkerThreadTaskRunner::current() [1] When the work is done, it calls PostTask() on this object. WebWorker threads however have an indeterminate lifetime. They can be aborted while the slow crypto operation is running on the crypto thread. By the time the crypto operation finishes on the crypto thread, and tries to post back to the origin blink thread (which was a WebWorker thread), that thread may no longer exist. In this scenario, the PostTask() fails and returns false. The Closure which was passed to PostTask (and was supposed to run on the origin blink thread) is deleted immediately from the calling thread (the crypto thread). This Closure owned the CryptoResultImpl, so now that is deleted rather than being leaked. There is no possibility of deleting the CryptoResultImpl anymore from the origin blink thread. This is why CryptoResultImpl is very careful not to hold onto any state tied to Blink, because it's destructor *may* be called on a different thread. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/child/work...
Patchset #2 (id:40001) has been deleted
Thank you for reviewing. I updated the patch according to discussion with yhirano. (Thank you, yhirano) My final goal is to make ScriptPromiseResolver RefCountedWillBeRefCountedGarbageCollected. c.f. https://codereview.chromium.org/783423003/
https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:198: // Creating the WeakResolver may return nullptr if active dom objects have This comment is no longer correct, and I believe the line below can now result in a use-after-free. The constructor of ScriptPromiseResolver may set m_state = ResolvedOrRejected When active DOM obects are already stopped. If that happens, then the call to keepAliveWhilePending() will not add a reference. So m_resolver will be set to the value of the WeakResolver, however as soon as the temporary PassRefPtr is destructed, the underlying object's refcount will reach zero and be freed. https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:74: void clearResolver(); Make this function private.
https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:72: CryptoResultImpl* m_result; I'm not sure this change is correct, because I think |m_result| can be destructed before this object is destroyed without RefPtr. Perhaps we should clear m_result explicitly when calling resolve / reject, right? https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:77: class WeakResolver; WeakResolver is no long "weak", so just Resolver is enough? https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:82: // Since ScriptPromiseResolver destroys itself, we should not use RefPtr here. Please wrap comments in 80 columns.
Thank you for review. https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:72: CryptoResultImpl* m_result; On 2014/12/11 02:27:59, yhirano wrote: > I'm not sure this change is correct, because I think |m_result| can be > destructed before this object is destroyed without RefPtr. > Perhaps we should clear m_result explicitly when calling resolve / reject, > right? I see. Now CryptoResultImpl::clearResolver() invokes clearCryptoResult() to clear m_result. https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:198: // Creating the WeakResolver may return nullptr if active dom objects have On 2014/12/09 19:15:29, eroman wrote: > This comment is no longer correct, and I believe the line below can now result > in a use-after-free. > > The constructor of ScriptPromiseResolver may set > m_state = ResolvedOrRejected > When active DOM obects are already stopped. > > If that happens, then the call to keepAliveWhilePending() will not add a > reference. So m_resolver will be set to the value of the WeakResolver, however > as soon as the temporary PassRefPtr is destructed, the underlying object's > refcount will reach zero and be freed. I see. I checked whether ActiveDOMObjectStopped here. If stopped, m_result = nullptr. https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:74: void clearResolver(); On 2014/12/09 19:15:29, eroman wrote: > Make this function private. Done. https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:77: class WeakResolver; On 2014/12/11 02:27:59, yhirano wrote: > WeakResolver is no long "weak", so just Resolver is enough? Done. https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:82: // Since ScriptPromiseResolver destroys itself, we should not use RefPtr here. On 2014/12/11 02:27:59, yhirano wrote: > Please wrap comments in 80 columns. Done.
lgtm https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:74: private: By making Resolver this class's inner class and having m_resolver as RawPtr<Resolver>, you can save friend declaration and down cast.
Let me ask a couple of questions about the lifetime to understand it better :) https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:51: class CryptoResultResolver : public ScriptPromiseResolver { Add final. https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:55: RefPtr<CryptoResultResolver> p = adoptRef(new CryptoResultResolver(scriptState, result)); p => resolver https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:80: CryptoResultImpl* m_result; Help me understand: Why can't this a RefPtr? https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:114: { Add ASSERT(!m_resolver) to verify that the resolver is cleared before the CryptoResultImpl is destructed. https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:210: ASSERT(scriptState); scriptState should never be 0. Instead you should check if scriptState->contextIsValid(). https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:80: // Since ScriptPromiseResolver destroys itself, we should not use RefPtr Just help me understand: Where is the ScriptPromiseResolver destroyed? https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:85: RawPtr<ScriptPromiseResolver> m_resolver; RawPtr<ScriptPromiseResolver> => ScriptPromiseResolver*
Thank you for review. https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:51: class CryptoResultResolver : public ScriptPromiseResolver { On 2014/12/12 00:04:10, haraken wrote: > > Add final. Done. https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:55: RefPtr<CryptoResultResolver> p = adoptRef(new CryptoResultResolver(scriptState, result)); On 2014/12/12 00:04:10, haraken wrote: > > p => resolver Done. https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.cpp:80: CryptoResultImpl* m_result; On 2014/12/12 00:04:10, haraken wrote: > > Help me understand: Why can't this a RefPtr? As far as I investigated, CryptoResultImpl should live longer than this Resolver. So Resolver don't need to use RefPtr to hold CryptoResultImpl. Instead, we should carefully clear m_result. https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:74: private: On 2014/12/11 12:28:41, yhirano wrote: > By making Resolver this class's inner class and having m_resolver as > RawPtr<Resolver>, you can save friend declaration and down cast. Done. https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:80: // Since ScriptPromiseResolver destroys itself, we should not use RefPtr On 2014/12/12 00:04:11, haraken wrote: > > Just help me understand: Where is the ScriptPromiseResolver destroyed? ScriptPromiseResolver destorys itself. So basically we should not hold ScriptPromiseResolver. https://codereview.chromium.org/780793002/diff/80001/Source/modules/crypto/Cr... Source/modules/crypto/CryptoResultImpl.h:85: RawPtr<ScriptPromiseResolver> m_resolver; On 2014/12/12 00:04:10, haraken wrote: > > RawPtr<ScriptPromiseResolver> => ScriptPromiseResolver* Done.
https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.cpp:121: { Add ASSERT(!m_resolver) to verify that m_resolver has been gone before CryptoResultImpl gets destructed. https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.cpp:225: ASSERT(scriptState); ASSERT(scriptState) => ASSERT(scriptState->contextIsValid()) https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:81: // Since ScriptPromiseResolver destroys itself, we should not use RefPtr I still don't fully understand why we cannot make it a RefPtr<ScriptPromiseResolver>. As far as I understand, ScriptPromiseResolver doesn't destroy itself but just deref()'s itself when the ScriptPromiseResolver is cleared. ScriptPromiseResolver is destroyed when the last reference is gone. So I think we can just make this a RefPtr<ScriptPromiseResolver> and clear the RefPtr in clearResolver(). It seems safer than having a raw pointer to the ScriptPromiseResolver. https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:82: // here. So to avoid using stale pointer, m_resolver should be cleared a stale pointer https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:83: // after resolving. after resolving => when getting resolved https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:86: ScriptPromiseResolver* m_resolver; Shall we change ScriptPromiseResolver* to CryptoResultImpl::Resolver* ? (But I agree that a right way to fix this problem is to change the ScriptPromiseResolver* to a ScriptPromise. We should try this in a follow-up CL.)
https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:81: // Since ScriptPromiseResolver destroys itself, we should not use RefPtr On 2014/12/12 08:49:31, haraken wrote: > > I still don't fully understand why we cannot make it a > RefPtr<ScriptPromiseResolver>. As far as I understand, ScriptPromiseResolver > doesn't destroy itself but just deref()'s itself when the ScriptPromiseResolver > is cleared. ScriptPromiseResolver is destroyed when the last reference is gone. > > So I think we can just make this a RefPtr<ScriptPromiseResolver> and clear the > RefPtr in clearResolver(). It seems safer than having a raw pointer to the > ScriptPromiseResolver. > I had some concerns on this explained in earlier comments: CryptoResultImpl may be deleted from a different thread. ScriptPromiseResolver is not RefCountedThreadSafe. Therefore it is misleading to have a RefPtr<> since it should not be releasing a reference during the destructor
https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:81: // Since ScriptPromiseResolver destroys itself, we should not use RefPtr On 2014/12/12 23:32:54, eroman wrote: > On 2014/12/12 08:49:31, haraken wrote: > > > > I still don't fully understand why we cannot make it a > > RefPtr<ScriptPromiseResolver>. As far as I understand, ScriptPromiseResolver > > doesn't destroy itself but just deref()'s itself when the > ScriptPromiseResolver > > is cleared. ScriptPromiseResolver is destroyed when the last reference is > gone. > > > > So I think we can just make this a RefPtr<ScriptPromiseResolver> and clear the > > RefPtr in clearResolver(). It seems safer than having a raw pointer to the > > ScriptPromiseResolver. > > > > I had some concerns on this explained in earlier comments: > > CryptoResultImpl may be deleted from a different thread. ScriptPromiseResolver > is not RefCountedThreadSafe. Therefore it is misleading to have a RefPtr<> since > it should not be releasing a reference during the destructor Thanks, makes sense. tasak@: Let's update a comment accordingly. BTW, it still looks strange that CryptoResultImpl and CryptoResultImpl::Resolver are referencing to each other using a raw pointer. Should we make CryptoResultImpl::Resolver::m_result a RefPtr<CryptoResultImpl>? (In the previous code, CryptoResultImpl::Resolver::m_result was a RefPtr<CryptoResultImpl>. We can just keep it.)
https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.cpp:127: static_cast<CryptoResultImpl::Resolver*>(m_resolver)->clearCryptoResult(); I don't think this is needed once you make CryptoResultImpl::Resolver a RefPtr<CryptoResultImpl>. You can remove clearCryptoResult.
Thank you for review. https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.cpp:121: { On 2014/12/12 08:49:31, haraken wrote: > > Add ASSERT(!m_resolver) to verify that m_resolver has been gone before > CryptoResultImpl gets destructed. > Done. https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.cpp:127: static_cast<CryptoResultImpl::Resolver*>(m_resolver)->clearCryptoResult(); On 2014/12/15 01:36:51, haraken wrote: > > I don't think this is needed once you make CryptoResultImpl::Resolver a > RefPtr<CryptoResultImpl>. You can remove clearCryptoResult. Done. https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.cpp:225: ASSERT(scriptState); On 2014/12/12 08:49:31, haraken wrote: > > ASSERT(scriptState) => ASSERT(scriptState->contextIsValid()) Done. https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/780793002/diff/100001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:83: // after resolving. On 2014/12/12 08:49:31, haraken wrote: > > after resolving => when getting resolved > Updated the comment.
LGTM
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/780793002/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187118 |
