Skip to content
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

Loading CSS in a background tab is flaky #3

Closed
cburgmer opened this issue Aug 24, 2014 · 10 comments
Closed

Loading CSS in a background tab is flaky #3

cburgmer opened this issue Aug 24, 2014 · 10 comments

Comments

@cburgmer
Copy link
Owner

Loading CSS resources over a HTTP source in a background tab in Firefox seems flaky. The error "Unable to load stylesheet undefined" is returned in some cases. Further debugging around the message makes the message disappear.

cssSupport.changeFontFaceRuleSrc() seems to have problems either retrieving rule.style.getPropertyValue("font-family") or rule.parentStyleSheet, the latter being null.

@SamHasler
Copy link

I get the Unable to load stylesheet undefined with some third party css that uses an @page rule.

I had to add console.log("CSS ERROR", a); before e.push({resourceType:"stylesheet",url:a.url,msg:"Unable to load stylesheet "+a.url} to be able to see where the error was.

@cburgmer
Copy link
Owner Author

cburgmer commented Sep 2, 2015

Hey Sam, if this is related to the @page rule and does not crop up in a background tab, then this is probably worth a separate report.

@SamHasler
Copy link

I've created an issue for it: #5

@Que3216
Copy link

Que3216 commented Feb 19, 2016

A similar bug happens on Chrome too: rule.parentStyleSheet is null in cssSupport.changeFontFaceRuleSrc(). It only happens intermittently, but the bug can be reliably reproduced by putting in a delay, which makes me think it could be a concurrency problem of some sort.

var iterateOverRulesAndInlineFontFace = function (cssRules, options) {
    var rulesToInline = findFontFaceRules(cssRules),
        errors = [],
        hasChanges = false;

    return util.all(rulesToInline.map(function (rule) {
        var srcDeclarationValue = rule.style.getPropertyValue("src");

        return loadAndInlineFontFace(srcDeclarationValue, options).then(function (result) {
            var defer = ayepromise.defer();
            // *** FORCE THE BUG BY ADDING IN A DELAY ***
            window.setTimeout(function () { defer.resolve(result); }, 10000);
            return defer.promise;
        }).then(function (result) {
            if (result.hasChanges) {
                cssSupport.changeFontFaceRuleSrc(cssRules, rule, result.srcDeclarationValue);

                hasChanges = true;
            }

            errors = errors.concat(result.errors);
        });
    })).then(function () {
        return {
            hasChanges: hasChanges,
            errors: errors
        };
    });
};

If you add breakpoints then you'll see that:

  • When the function enters parentStyleSheet is correctly defined on all rules in cssRules
  • When the call to cssSupport.changeFontFaceRuleSrc is made, parentStyleSheet is null on ALL rules in cssRules

Could it be that some other Promise is completing before this one, and detaching the cssRules from the parent style sheet? Is there any code that does this?

Any promise called from within a util.all call, which touches the css rules/stylesheet could potentially cause this.

@Que3216
Copy link

Que3216 commented Feb 19, 2016

Updating exchangeRule in cssSupport as follows fixed the problem:

Before:

exports.exchangeRule = function (cssRules, rule, newRuleText) {
    var ruleIdx = cssRules.indexOf(rule);
    //    styleSheet = rule.parentStyleSheet;

    // Generate a new rule
    styleSheet.insertRule(newRuleText, ruleIdx+1);
    styleSheet.deleteRule(ruleIdx);
    // Exchange with the new
    cssRules[ruleIdx] = styleSheet.cssRules[ruleIdx];
};

After:

exports.exchangeRule = function (cssRules, rule, newRuleText) {
    var ruleIdx = cssRules.indexOf(rule);
    cssRules[ruleIdx] = exports.rulesForCssText(newRuleText)[0];
};

There doesn't seem to be much of a perf hit at all. However this is more a fix for the symptom than the root cause.

Que3216 pushed a commit to Que3216/inlineresources that referenced this issue Feb 19, 2016
… sheet for the parsing, instead of using the parentStyleSheet property
@cburgmer
Copy link
Owner Author

Thanks for the thorough debugging and the clean write up.
Let's hope this also helps for Firefox.

Can you let me know what environment your are testing in? With the proposed delay proposed above it only makes Jasmine run into timeouts for me.

@cburgmer
Copy link
Owner Author

As you suggest, a better analysis on what happens in parallel in the background might help understanding the effects. However your change helps decouple us from references inside the DOM structure, so this feels definitely safer.

@Que3216
Copy link

Que3216 commented Feb 22, 2016

Hi Chris,

Thanks for the quick response and merging this in! I tested it via rasterizeHtml installed in a larger application, rather than by running the unit tests.

Would it be possible to bubble this up to rasterizeHtml and create a new release? I can create a PR to bump the version in rasterizeHtml if you like.

Quentin

@cburgmer
Copy link
Owner Author

cburgmer commented Feb 22, 2016 via email

@cburgmer
Copy link
Owner Author

Fixed for good it seems. Closing

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

3 participants