search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] Fix src/xorigin.js to not use function statement that isn't top-level and break strict mode when minifying #9415

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by humphd Tuesday Mar 03, 2015 at 20:41 GMT Originally opened as https://github.com/adobe/brackets/pull/10683

Working on getting Brackets to run well in-browser, I'm playing with various minificaiton strategies. While doing this I hit an issue where xorigin.js was breaking strict mode. This fixes it.

See http://whereswalden.com/2011/01/24/new-es5-strict-mode-requirement-function-statements-not-at-top-level-of-a-program-or-function-are-prohibited/ for details on why this is bad.

humphd included the following code: https://github.com/adobe/brackets/pull/10683/commits

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday Mar 03, 2015 at 22:14 GMT

I'm confused... This function is at the top level of its scope (not nested inside some other block construct like an if), and this module has "use strict" on it, so if this were a violation of strict mode this code wouldn't even run at all.

Do you know what Uglify transforms this code to that causes it to break? Maybe looking at the diff would make things more clear...

core-ai-bot commented 3 years ago

Comment by humphd Wednesday Mar 04, 2015 at 15:28 GMT

The problem isn't what's there now, it's how what's there gets transformed by more aggressive minification. By doing the change I have in this PR, you end up with this (I've expanded it):

!function() {
    "use strict";
    var testCrossOriginError;
    if (-1 !== navigator.userAgent.search(" Chrome/") ? testCrossOriginError = function(message, url, line) {
        return "" === url && 0 === line && "Script error." === message
    } : "Opera/" === navigator.userAgent.slice(0, 6) && (testCrossOriginError = function(message) {
        return "Uncaught exception: DOMException: NETWORK_ERR" === message
    }), "undefined" == typeof brackets && "file://" === document.location.href.substr(0, 7) && testCrossOriginError) {
        var previousErrorHandler = window.onerror, handleError = function(message, url, line) {
            if (testCrossOriginError(message, url, line))
                alert("Oops! This application doesn't run in browsers yet.\n\nIt is built in HTML, but right now it runs as a desktop app so you can use it to edit local files. Please use the application shell in the following repo to run this application:\n\ngithub.com/adobe/brackets-shell"), window.onerror = previousErrorHandler;
            else if (previousErrorHandler)
                return previousErrorHandler(message, url, line)
        };
        window.onerror = handleError
    }
}();

So this change is innocuous in the case that Brackets' source is just used as is, but necessary when minifiying the code and having the AST transformed.

core-ai-bot commented 3 years ago

Comment by humphd Wednesday Mar 04, 2015 at 15:52 GMT

For reference, what happens when I don't change it:

!function() {
    "use strict";
    var testCrossOriginError;
    if (-1 !== navigator.userAgent.search(" Chrome/"))
        testCrossOriginError = function(message, url, line) {
            return "" === url && 0 === line && "Script error." === message
        };
    else if ("Opera/" === navigator.userAgent.slice(0, 6))
        testCrossOriginError = function(message) {
            return "Uncaught exception: DOMException: NETWORK_ERR" === message
        };
    if ("undefined" == typeof brackets && "file://" === document.location.href.substr(0, 7) && testCrossOriginError) {
        var previousErrorHandler = window.onerror;
        function handleError(message, url, line) {
            if (testCrossOriginError(message, url, line))
                alert("Oops! This application doesn't run in browsers yet.\n\nIt is built in HTML, but right now it runs as a desktop app so you can use it to edit local files. Please use the application shell in the following repo to run this application:\n\ngithub.com/adobe/brackets-shell"), window.onerror = previousErrorHandler;
            else if (previousErrorHandler)
                return previousErrorHandler(message, url, line)
        }
        window.onerror = handleError
    }
}();

It's because that final if case gets expanded to hold all the rest of the code.

core-ai-bot commented 3 years ago

Comment by peterflynn Wednesday Mar 04, 2015 at 22:29 GMT

This seems like a bug in the minifier then -- if it can't reliably transform strict-complying code without sometimes breaking strict mode. Since we use nested functions tons of places in the Brackets codebase, we could easily hit the same bug in other parts of the codebase without warning...

core-ai-bot commented 3 years ago

Comment by humphd Thursday Mar 05, 2015 at 00:34 GMT

I looked at it a bit further, and it's not a bug in uglify. It's happening because I'm explicitly turning off function hoisting in order to get the extensions to work properly (it seems that the text plugin loading breaks if that's on). So it's whack a mole here :(