Skip to content

Regression in modularize mode (currentSrc issue) #6903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bvibber opened this issue Jul 25, 2018 · 15 comments
Closed

Regression in modularize mode (currentSrc issue) #6903

bvibber opened this issue Jul 25, 2018 · 15 comments

Comments

@bvibber
Copy link
Collaborator

bvibber commented Jul 25, 2018

Compiling ogv.js with emscripten sdk 1.38.10, I'm seeing a new failure mode with my demuxer module, which is loaded asynchronously into the web page context:

[Error] TypeError: null is not an object (evaluating 'ca.src')

This is in the (minified) setup code:

        if (n) {
            var da = this._currentScript || document.currentScript;
            0 !== da.src.indexOf("blob:") && (t = da.src.split("/").slice(0, -1).join("/") +
            "/")
        } else

source:

   if (ENVIRONMENT_IS_WEB) {
#if MODULARIZE
    // When MODULARIZE, this JS may be executed later, after document.currentScript is gone, so we send it
    // using this._currentScript.
#else
    var currentScript = document.currentScript;
#endif
     if (currentScript.src.indexOf('blob:') !== 0) {
       scriptDirectory = currentScript.src.split('/').slice(0, -1).join('/') + '/';
     }

Something about this isn't doing what I expect...

I notice at the end of the .js file is this which appears to be an attempt to capture the currentSource:

OGVDemuxerWebMW = OGVDemuxerWebMW.bind({
    _currentScript: typeof document !== 'undefined' ? document.currentScript : undefined
});

But that syntax for the Function.prototype.bind function doesn't appear to be documented on MDN, and it doesn't seem to do anything that I can tell on Safari 11.1.2.

@bvibber
Copy link
Collaborator Author

bvibber commented Jul 25, 2018

That bit appears to have come from d1f303a

@kripken
Copy link
Member

kripken commented Jul 25, 2018

Odd, isn't that bind usage basically the simple case shown here:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind

(of binding an object which is then this in the call)? Does that MDN example not work in Safari?

@kripken
Copy link
Member

kripken commented Jul 25, 2018

If that's not it, is it possible that the line with OGVDemuxerWebMW = OGVDemuxerWebMW.bind({ .. }) is not being called, somehow? If so, and it's calling the unbound version, then it would see a null there, which would seem to fit the symptoms (but I have no idea why that line wouldn't be reached, very odd).

@nazar-pc
Copy link
Contributor

I suspect it is called, but asynchronously as part of a bigger bundle. You may want to adjust your build process or override Module.locateFile. On Emscripten side we should probably add a check whether currentScript is an object at all to avoid this error.

@kripken
Copy link
Member

kripken commented Jul 25, 2018

Hmm, we do the bind right after defining the function, so it seems like that has to happen.

And we do check if this['_currentScript'] exists, so I think that should handle the case of not having document or document.currentScript defined during binding.

Maybe another binding happens later, overriding this one?

@kripken
Copy link
Member

kripken commented Jul 25, 2018

Btw, this code slightly changed since the original landing - @Brion is this still an issue on latest incoming?

@bvibber
Copy link
Collaborator Author

bvibber commented Jul 25, 2018

(document and document.currentScript are defined at the time that bind() is called, I checked in the debugger)

@bvibber
Copy link
Collaborator Author

bvibber commented Jul 25, 2018

Ah, I understand what the bind() call is doing now -- it's putting the _currentScript property on the hidden this, not on the Module object... investigating further on my end.

@bvibber
Copy link
Collaborator Author

bvibber commented Jul 25, 2018

AHA!

My call to the OGVDemuerWebMW module function was made like this:

return new global[className](options);

which used to work fine. That "new" replaces the "this" parameter from the bind() call with a copy of the prototype, which has no _currentScript property.

Removing the "new" from the calls should get it working... recompiling now. :)

@bvibber
Copy link
Collaborator Author

bvibber commented Jul 25, 2018

Aaaaand it works now. :) As far as I know following documentation correctly in the initial call would have saved me here. :D

@bvibber bvibber closed this as completed Jul 25, 2018
@kripken
Copy link
Member

kripken commented Jul 25, 2018

Great, thanks for the update @Brion!

I added a commit to #6894 to avoid crashing in either the case of document.scriptDirectory not existing (as @nazar-pc mentioned), or doing new Module() which overrides the object containing the helper property. In both cases we can't access the scriptDirectory but we should at least not crash.

kripken added a commit that referenced this issue Jul 25, 2018
* handle the case of a missing document.currentScript, and of the MODULARIZE .bind() being overridden by `new` (see discussion in #6903). in both cases we can't find the scriptDirectory, but we should at least not crash.

* generalize blob url handling in scriptDirectory computation to also handle workers
kripken added a commit that referenced this issue Jul 28, 2018
*    Support  new Module()  (instead of just  Module() ) in finding the scriptDirectory. This is done by avoiding .bind(), and saving it in a closure. This seems like a potential common pitfall, so worth supporting even if it isn't the documented use (see #6903).
*    Don't emit the _scriptDir code for MODULARIZE_INSTANCE - we only need it in MODULARIZE.
*    Refactor the emcc.py code, do the extra scriptDirectory or instance stuff later, which I think is clearer.
*    Add testing for MODULARIZE_INSTANCE and scriptDirectory detection.
@tgds
Copy link

tgds commented Nov 9, 2018

@kripken this introduces a new problem. document.currentScript.src is undefined if you import the script in SVG using <script href="a.out.js"> and the script crashes with Cannot read property 'indexOf' of undefined

@kripken
Copy link
Member

kripken commented Nov 12, 2018

Thanks @tgds. I hadn't realized that could be an issue. I filed #7494

What's the use case for loading in an SVG, by the way?

@tgds
Copy link

tgds commented Nov 13, 2018

Honestly, I only came across it in this Google codelab and that's how I found the issue. I suppose the use case is interactive SVGs sharable without HTML glue?

@kripken
Copy link
Member

kripken commented Nov 13, 2018

Interesting, thanks @tgds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants