Don't assert when ServiceWorker::from is passed an empty promise

Source/bindings/v8/ScriptPromise.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 49 matching lines @@
     if (!value->IsPromise()) {
         m_promise = ScriptValue(scriptState, v8::Handle<v8::Value>());
         V8ThrowException::throwTypeError("the given value is not a Promise", scriptState->isolate());
         return;
     }
     m_promise = ScriptValue(scriptState, value);
 }
 
 ScriptPromise ScriptPromise::then(PassOwnPtr<ScriptFunction> onFulfilled, PassOwnPtr<ScriptFunction> onRejected)
 {
+    v8::Local<v8::Function> v8OnFulfilled = ScriptFunction::adoptByGarbageCollector(onFulfilled);
+    v8::Local<v8::Function> v8OnRejected = ScriptFunction::adoptByGarbageCollector(onRejected);

[haraken, 2014/06/17 00:51:55]

Would you elaborate on this change? It seems a bit strange to me that just
adopting ScriptFunctions addresses some issue. Also in a case where a
ScriptPromise is empty, I wonder why we need to adopt ScriptFunctions.

[falken, 2014/06/17 01:07:51]

The destructor of PassOwnPtr deletes the ScriptFunction, but ScriptFunction
inherits V8GarbageCollected whose dtor will ASSERT that m_handle was set. AFAICT
the way to set m_handle is to call adoptbyGarbageCollector.

So I think the caller expects ScriptPromise::then to always adopt the args.

[haraken, 2014/06/17 01:15:25]

Thanks for the details; that ASSERT doesn't make much sense to me. Can we change
the ASSERT to something like ASSERT(alreadyAdopted || m_handle) ?

+
     if (m_promise.isEmpty())
         return ScriptPromise();
 
     v8::Local<v8::Object> promise = m_promise.v8Value().As<v8::Object>();
-    v8::Local<v8::Function> v8OnFulfilled = ScriptFunction::adoptByGarbageCollector(onFulfilled);
-    v8::Local<v8::Function> v8OnRejected = ScriptFunction::adoptByGarbageCollector(onRejected);
-
     ASSERT(promise->IsPromise());
     // Return this Promise if no handlers are given.
     // In fact it is not the exact bahavior of Promise.prototype.then
     // but that is not a problem in this case.
     v8::Local<v8::Promise> resultPromise = promise.As<v8::Promise>();
     // FIXME: Use Then once it is introduced.
     if (!v8OnFulfilled.IsEmpty()) {
         resultPromise = resultPromise->Chain(v8OnFulfilled);
         if (resultPromise.IsEmpty()) {
             // v8::Promise::Chain may return an empty value, for example when
@@ skipping 40 matching lines @@
     return promise;
 }
 
 ScriptPromise ScriptPromise::rejectWithDOMException(ScriptState* scriptState, PassRefPtrWillBeRawPtr<DOMException> exception)
 {
     ASSERT(scriptState->isolate()->InContext());
     return reject(scriptState, ToV8Value<WithScriptState, v8::Handle<v8::Object> >::toV8Value(exception, scriptState->context()->Global(), scriptState->isolate()));
 }
 
 } // namespace WebCore