cocos2d/cocos2d-js#1658: Fix js_load_remote_image issue with multithread

cocos/scripting/js-bindings/manual/extension/jsb_cocos2dx_extension_manual.cpp

@@ -908,7 +908,7 @@ bool js_cocos2dx_ext_release(JSContext *cx, uint32_t argc, jsval *vp)
 }
 
 
-__JSDownloaderDelegator::__JSDownloaderDelegator(JSContext *cx, JS::HandleObject obj, const std::string &url, JS::HandleValue callback)
+__JSDownloaderDelegator::__JSDownloaderDelegator(JSContext *cx, JS::HandleObject obj, const std::string &url, JS::HandleObject callback)
 : _cx(cx)
 , _url(url)
 , _buffer(nullptr)
@@ -917,7 +917,27 @@ __JSDownloaderDelegator::__JSDownloaderDelegator(JSContext *cx, JS::HandleObject
     _obj.ref().set(obj);
     _jsCallback.construct(_cx);
     _jsCallback.ref().set(callback);
-
+}
+
+__JSDownloaderDelegator::~__JSDownloaderDelegator()
+{
+    _obj.destroyIfConstructed();
+    _jsCallback.destroyIfConstructed();
+    if (_buffer != nullptr)
+        free(_buffer);
+    _downloader->setErrorCallback(nullptr);
+    _downloader->setSuccessCallback(nullptr);
+}
+
+__JSDownloaderDelegator *__JSDownloaderDelegator::create(JSContext *cx, JS::HandleObject obj, const std::string &url, JS::HandleObject callback)
+{
+    __JSDownloaderDelegator *delegate = new (std::nothrow) __JSDownloaderDelegator(cx, obj, url, callback);
+    delegate->autorelease();
+    return delegate;
+}
+
+void __JSDownloaderDelegator::startDownload()
+{
     if (Director::getInstance()->getTextureCache()->getTextureForKey(_url))
     {
         onSuccess(nullptr, nullptr, nullptr);
@@ -929,8 +949,9 @@ __JSDownloaderDelegator::__JSDownloaderDelegator(JSContext *cx, JS::HandleObject
         _downloader->setErrorCallback( std::bind(&__JSDownloaderDelegator::onError, this, std::placeholders::_1) );
         _downloader->setSuccessCallback( std::bind(&__JSDownloaderDelegator::onSuccess, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3) );
 
-        long contentSize = _downloader->getContentSize(_url);
-        if (contentSize == -1) {
+        cocos2d::extension::Downloader::HeaderInfo info = _downloader->getHeader(_url);
+        long contentSize = info.contentSize;
+        if (contentSize == -1 || info.responseCode >= 400) {
             cocos2d::extension::Downloader::Error err;
             onError(err);
         }
@@ -942,94 +963,101 @@ __JSDownloaderDelegator::__JSDownloaderDelegator(JSContext *cx, JS::HandleObject
     }
 }
 
-__JSDownloaderDelegator::~__JSDownloaderDelegator()
+void __JSDownloaderDelegator::download()
 {
-    if (_buffer != nullptr)
-        free(_buffer);
-    _downloader->setErrorCallback(nullptr);
-    _downloader->setSuccessCallback(nullptr);
+    retain();
+    startDownload();
+}
+
+void __JSDownloaderDelegator::downloadAsync()
+{
+    retain();
+    auto t = std::thread(&__JSDownloaderDelegator::startDownload, this);
+    t.detach();
 }
 
 void __JSDownloaderDelegator::onError(const cocos2d::extension::Downloader::Error &error)
 {
-    if (!_jsCallback.ref().isNull()) {
-        JSContext *cx = ScriptingCore::getInstance()->getGlobalContext();
-        JS::RootedObject global(cx, ScriptingCore::getInstance()->getGlobalObject());
-
-        JSAutoCompartment ac(_cx, _obj.ref());
-
-        jsval succeed = BOOLEAN_TO_JSVAL(false);
-        JS::RootedValue retval(cx);
-        JS_CallFunctionValue(cx, global, _jsCallback.ref(), JS::HandleValueArray::fromMarkedLocation(1, &succeed), &retval);
-    }
-    this->release();
+    Director::getInstance()->getScheduler()->performFunctionInCocosThread([this]
+    {
+        JS::RootedValue callback(_cx, OBJECT_TO_JSVAL(_jsCallback.ref()));
+        if (!callback.isNull()) {
+            JS::RootedObject global(_cx, ScriptingCore::getInstance()->getGlobalObject());
+            JSAutoCompartment ac(_cx, global);
+
+            jsval succeed = BOOLEAN_TO_JSVAL(false);
+            JS::RootedValue retval(_cx);
+            JS_CallFunctionValue(_cx, global, callback, JS::HandleValueArray::fromMarkedLocation(1, &succeed), &retval);
+        }
+        release();
+    });
 }
 
 void __JSDownloaderDelegator::onSuccess(const std::string &srcUrl, const std::string &storagePath, const std::string &customId)
 {
     Image *image = new Image();
-    jsval valArr[2];
-    JSContext *cx = ScriptingCore::getInstance()->getGlobalContext();
-    JS::RootedObject global(cx, ScriptingCore::getInstance()->getGlobalObject());
     cocos2d::TextureCache *cache = Director::getInstance()->getTextureCache();
 
-    JSAutoCompartment ac(_cx, _obj.ref() ? _obj.ref() : global);
-
     Texture2D *tex = cache->getTextureForKey(_url);
-    if (tex)
+    if (!tex)
     {
-        valArr[0] = BOOLEAN_TO_JSVAL(true);
-        js_proxy_t* p = jsb_get_native_proxy(tex);
-        valArr[1] = OBJECT_TO_JSVAL(p->obj);
+        if (image->initWithImageData(_buffer, _size))
+        {
+            tex = Director::getInstance()->getTextureCache()->addImage(image, _url);
+        }
     }
-    else if (image->initWithImageData(_buffer, _size))
+    image->release();
+
+    Director::getInstance()->getScheduler()->performFunctionInCocosThread([this, tex]
     {
-        tex = Director::getInstance()->getTextureCache()->addImage(image, _url);
-        valArr[0] = BOOLEAN_TO_JSVAL(true);
+        JS::RootedObject global(_cx, ScriptingCore::getInstance()->getGlobalObject());
+        JSAutoCompartment ac(_cx, global);
 
-        JS::RootedObject texProto(cx, jsb_cocos2d_Texture2D_prototype);
-        JSObject *obj = JS_NewObject(cx, jsb_cocos2d_Texture2D_class, texProto, global);
-        // link the native object with the javascript object
-        js_proxy_t* p = jsb_new_proxy(tex, obj);
-        JS::AddNamedObjectRoot(cx, &p->obj, "cocos2d::Texture2D");
-        valArr[1] = OBJECT_TO_JSVAL(p->obj);
-    }
-    else
-    {
-        valArr[0] = BOOLEAN_TO_JSVAL(false);
-        valArr[1] = JSVAL_NULL;
-    }
-
-    image->release();
+        jsval valArr[2];
+        if (tex)
+        {
+            valArr[0] = BOOLEAN_TO_JSVAL(true);
+            js_proxy_t* p = jsb_get_native_proxy(tex);
+            if (!p)
+            {
+                JS::RootedObject texProto(_cx, jsb_cocos2d_Texture2D_prototype);
+                JSObject *obj = JS_NewObject(_cx, jsb_cocos2d_Texture2D_class, texProto, global);
+                // link the native object with the javascript object
+                p = jsb_new_proxy(tex, obj);
+                JS::AddNamedObjectRoot(_cx, &p->obj, "cocos2d::Texture2D");
+            }
+            valArr[1] = OBJECT_TO_JSVAL(p->obj);
+        }
+        else
+        {
+            valArr[0] = BOOLEAN_TO_JSVAL(false);
+            valArr[1] = JSVAL_NULL;
+        }
 
-    if (!_jsCallback.ref().isNull()) {
-        JS::RootedValue retval(cx);
-        JS_CallFunctionValue(cx, global, _jsCallback.ref(), JS::HandleValueArray::fromMarkedLocation(2, valArr), &retval);
-    }
-    this->release();
-}
-
-void __JSDownloaderDelegator::download(JSContext *cx, JS::HandleObject obj, const std::string &url, JS::HandleValue callback)
-{
-    auto t = std::thread([cx, obj, url, callback]() {
-        new __JSDownloaderDelegator(cx, obj, url, callback);
+        JS::RootedValue callback(_cx, OBJECT_TO_JSVAL(_jsCallback.ref()));
+        if (!callback.isNull())
+        {
+            JS::RootedValue retval(_cx);
+            JS_CallFunctionValue(_cx, global, callback, JS::HandleValueArray::fromMarkedLocation(2, valArr), &retval);
+        }
+        release();
     });
-    t.detach();
 }
 
 // jsb.loadRemoteImg(url, function(succeed, result) {})
 bool js_load_remote_image(JSContext *cx, uint32_t argc, jsval *vp)
 {
     JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
     JS::RootedObject obj(cx, JS_THIS_OBJECT(cx, vp));
-    if (argc == 2) {
+    if (argc == 2)
+    {
         std::string url;
         bool ok = jsval_to_std_string(cx, args.get(0), &url);
-        JS::RootedValue callback(cx, args.get(1));
-
-        __JSDownloaderDelegator::download(cx, obj, url, callback);
+        JSB_PRECONDITION2(ok, cx, false, "js_load_remote_image : Error processing arguments");
+        JS::RootedObject callback(cx, args.get(1).toObjectOrNull());
 
-        JSB_PRECONDITION2(ok, cx, false, "js_console_log : Error processing arguments");
+        __JSDownloaderDelegator *delegate = __JSDownloaderDelegator::create(cx, obj, url, callback);
+        delegate->downloadAsync();
 
         args.rval().setUndefined();
         return true;

cocos/scripting/js-bindings/manual/extension/jsb_cocos2dx_extension_manual.h

@@ -32,12 +32,17 @@
 class __JSDownloaderDelegator : cocos2d::Ref
 {
 public:
-    static void download(JSContext *cx, JS::HandleObject obj, const std::string &url, JS::HandleValue callback);
+    void downloadAsync();
+    void download();
 
+    static __JSDownloaderDelegator *create(JSContext *cx, JS::HandleObject obj, const std::string &url, JS::HandleObject callback);
+
 protected:
-    __JSDownloaderDelegator(JSContext *cx, JS::HandleObject obj, const std::string &url, JS::HandleValue callback);
+    __JSDownloaderDelegator(JSContext *cx, JS::HandleObject obj, const std::string &url, JS::HandleObject callback);
     ~__JSDownloaderDelegator();
 
+    void startDownload();
+
 private:
     void onSuccess(const std::string &srcUrl, const std::string &storagePath, const std::string &customId);
     void onError(const cocos2d::extension::Downloader::Error &error);
@@ -46,8 +51,8 @@ class __JSDownloaderDelegator : cocos2d::Ref
     std::shared_ptr<cocos2d::extension::Downloader> _downloader;
     std::string _url;
     JSContext *_cx;
-    mozilla::Maybe<JS::RootedValue> _jsCallback;
-    mozilla::Maybe<JS::RootedObject> _obj;
+    mozilla::Maybe<JS::PersistentRootedObject> _jsCallback;
+    mozilla::Maybe<JS::PersistentRootedObject> _obj;
 };
 
 void register_all_cocos2dx_extension_manual(JSContext* cx, JS::HandleObject global);

extensions/assets-manager/Downloader.cpp

@@ -325,6 +325,11 @@ long Downloader::getContentSize(const std::string &srcUrl)
     return info.contentSize;
 }
 
+Downloader::HeaderInfo Downloader::getHeader(const std::string &srcUrl)
+{
+    return prepareHeader(srcUrl);
+}
+
 void Downloader::getHeaderAsync(const std::string &srcUrl, const HeaderCallback &callback)
 {
     setHeaderCallback(callback);
@@ -401,25 +406,26 @@ void Downloader::downloadToBuffer(const std::string &srcUrl, const std::string &
     CURLcode res = curl_easy_perform(curl);
     if (res != CURLE_OK)
     {
-        _fileUtils->removeFile(data.path + data.name + TEMP_EXT);
-        std::string msg = StringUtils::format("Unable to download file: [curl error]%s", curl_easy_strerror(res));
+        std::string msg = StringUtils::format("Unable to download file to buffer: [curl error]%s", curl_easy_strerror(res));
         this->notifyError(msg, customId, res);
     }
-
-    curl_easy_cleanup(curl);
-
-    Director::getInstance()->getScheduler()->performFunctionInCocosThread([=]{
-        if (!ptr.expired())
-        {
-            std::shared_ptr<Downloader> downloader = ptr.lock();
-
-            auto successCB = downloader->getSuccessCallback();
-            if (successCB != nullptr)
+    else
+    {
+        Director::getInstance()->getScheduler()->performFunctionInCocosThread([=]{
+            if (!ptr.expired())
             {
-                successCB(data.url, "", data.customId);
+                std::shared_ptr<Downloader> downloader = ptr.lock();
+
+                auto successCB = downloader->getSuccessCallback();
+                if (successCB != nullptr)
+                {
+                    successCB(data.url, "", data.customId);
+                }
             }
-        }
-    });
+        });
+    }
+
+    curl_easy_cleanup(curl);
 }
 
 void Downloader::downloadAsync(const std::string &srcUrl, const std::string &storagePath, const std::string &customId/* = ""*/)

extensions/assets-manager/Downloader.h

@@ -107,7 +107,7 @@ class CC_EX_DLL Downloader : public std::enable_shared_from_this<Downloader>
         std::string url;
         std::string contentType;
         double contentSize;
-        double responseCode;
+        long responseCode;
     };
 
     typedef std::unordered_map<std::string, DownloadUnit> DownloadUnits;
@@ -139,6 +139,8 @@ class CC_EX_DLL Downloader : public std::enable_shared_from_this<Downloader>
 
     long getContentSize(const std::string &srcUrl);
 
+    HeaderInfo getHeader(const std::string &srcUrl);
+
     void getHeaderAsync(const std::string &srcUrl, const HeaderCallback &callback);
 
     void downloadToBufferAsync(const std::string &srcUrl, unsigned char *buffer, const long &size, const std::string &customId = "");

tests/js-tests/src/TextureCacheTest/TextureCacheTest.js

@@ -192,7 +192,6 @@ var RemoteTextureTest = TextureCacheTestBase.extend({
     _title:"Remote Texture Test",
     _subtitle:"",
     _remoteTex: "http://cn.cocos2d-x.org/image/logo.png",
-    _sprite : null,
     onEnter:function () {
         this._super();
         if('opengl' in cc.sys.capabilities && !cc.sys.isNative){
@@ -205,19 +204,21 @@ var RemoteTextureTest = TextureCacheTestBase.extend({
     },
 
     startDownload: function() {
-        cc.textureCache.addImageAsync(this._remoteTex, this.texLoaded, this);
+        var imageUrlArray = ["http://www.cocos2d-x.org/s/upload/v35.jpg", "http://www.cocos2d-x.org/s/upload/testin.jpg", "http://www.cocos2d-x.org/s/upload/geometry_dash.jpg", "http://cn.cocos2d-x.org/image/logo.png"];
+
+        for (var i = 0; i < imageUrlArray.length; i++) {
+            cc.textureCache.addImageAsync(imageUrlArray[i], this.texLoaded, this);
+        }
     },
 
     texLoaded: function(texture) {
         if (texture instanceof cc.Texture2D) {
-            cc.log("Remote texture loaded: " + this._remoteTex);
-            if (this._sprite) {
-                this.removeChild(this._sprite);
-            }
-            this._sprite = new cc.Sprite(texture);
-            this._sprite.x = cc.winSize.width/2;
-            this._sprite.y = cc.winSize.height/2;
-            this.addChild(this._sprite);
+            cc.log("Remote texture loaded");
+
+            var sprite = new cc.Sprite(texture);
+            sprite.x = cc.winSize.width/2;
+            sprite.y = cc.winSize.height/2;
+            this.addChild(sprite);
         }
         else {
             cc.log("Fail to load remote texture");