693725 - Hang setting stylesheet attribute in its onload

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase (hangs Firefox)

This hangs and probably shouldn't.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  So what happens here is that nsHTMLStyleElement::SetAttr calls UpdateStyleSheetInternal which calls nsStyleLinkElement::DoUpdateStyleSheet.

Both calls pass aForceUpdate == false.  But aForceUpdate is ignored in the inline case, so we just call LoadInlineStyle, which synchronously parses the style data and adds the thread observer for the load event.  Then we unwind back to the event loop, but call the thread observer, etc.

Jonas, I could add various things to mitigate this (like not updating in nsHTMLStyleElement::SetAttr unless the attr is one that matters), but fundamentally the problem is that the load event for sheets can "look sync" in the sense of blocking event loop processing as long as the load event can run code that synchronously applies a stylesheet....

Note that restricting load events to non-inline sheets doesn't help either, since those can also get applied sync if they're cached, and that would cause a "sync" load event.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, so if I understand this correctly, what happens is basically someone writing code like:

window.addEventListener("myevent", function(e) {
  e = document.createEvent("Event");
  e.initEvent("myevent", false, false);
  window.dispatchEvent(e);
}, false);

I.e. an event listener for an event causing the event to happen again.

There are arguably two things that make the testcase in comment 0 worse than the above code:

1. It's not really clear that the action causes the event to be fired. I.e. it's easy to do by accident.
2. Due to the way the event is fired, the slow-script dialog isn't kicking in.

The low-hanging fruit here seems to be to fix 1. We should only fire the load event if someone actually changes the url of the stylesheet (for linked sheets) or the contents of it (for inline sheets).

Fixing 2 would be nice, but seems a lot harder and should probably be filed as a separate bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I.e. an event listener for an event causing the event to happen again.

Yes.  Though in the case in comment 2, I think our overrecursion protection would catch it...

And yes, your two points.  ;)

Firing just on url/content changes isn't quite right.  Changing @type can trigger a real sheet load/parse too....

So what set of changes should actually trigger the event?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I do think that we should fire the event if something's actually changed. So if you change @type from not existing to empty string or to "text/css", then practially speaking nothing changed, right?

Similarly, if you call set the url attribute to the same value again, we also should not fire a new load (though I think we have code in the attr setting code that short-cuts that anyway).

Could we, before we're about to start the process of loading a sheet and which eventually ends up firing "load", check if the current nsIURI is equivalent to the existing nsIURI, and if it is just bail?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> So if you change @type from not existing to empty string or to "text/css", then
> practially speaking nothing changed

Yes, though right now we don't keep track of that; we just reparse on type changes...

> Similarly, if you call set the url attribute to the same value again, we also should
> not fire a new load (though I think we have code in the attr setting code that
> short-cuts that anyway).

That's the aForceUpdate thing I mention.

Changes to type and media and title change the nsCSSStyleSheet object involved at the moment...

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Boris Zbarsky (:bz) from comment #5)
> > So if you change @type from not existing to empty string or to "text/css", then
> > practially speaking nothing changed
> 
> Yes, though right now we don't keep track of that; we just reparse on type
> changes...

But do we actually ever parse using different rules. Aren't we basically switching between "don't parse because it's a type we don't know" and "parse as CSS"?

> > Similarly, if you call set the url attribute to the same value again, we also should
> > not fire a new load (though I think we have code in the attr setting code that
> > short-cuts that anyway).
> 
> That's the aForceUpdate thing I mention.
> 
> Changes to type and media and title change the nsCSSStyleSheet object
> involved at the moment...

Sure, but it doesn't seem like any of those would need to trigger new network activity, right? Or do we hit the network for those changes for simplicities sake?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Aren't we basically switching between "don't parse because it's a type we don't know"
> and "parse as CSS"?

Yes.

> Sure, but it doesn't seem like any of those would need to trigger new network activity,
> right?

I _believe_ that with the current CSSLoader setup this is correct.

Comment 8

[Fabio Beltramini]

Although the testcase provided above now seems to run fine and no longer reproduces a hang for me (tested in both 33.0.3 and 34.0.5 on Windows7), I ran across it in this way:

    <html>
        <head>
          <link rel="stylesheet" media="later" onload="this.media='screen'" href="blank-file.css" />
        </head>
        <body></body>
     </html>

This hangs the browser, consumes a full CPU core and quickly consumes all available system memory.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Right, see comment 5...

Comment 10

[scott]

I ran into this today while making an update in loadCSS (https://github.com/filamentgroup/loadCSS/blob/74/loadCSS.js). I found I could avoid freezing the browser by placing the callback in a setTimeout, but that's not ideal.

Comment 11

[Mark Straver]

Looks like http://whatsmyuseragent.com has made a recent change that triggers this hang in Firefox as well.

        <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.3.0/css/font-awesome.min.css" media="none" onload="media='all'" />
        <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.11.4/jquery-ui.min.css" media="none" onload="media='all'" />

Effectively grinds the browser to a halt, without even rendering anything. pegs a core, and eats up memory until it OOM crashes. Any "media='value'" in a stylesheet <link> will cause DoS, AFAICT.

Comment 12

[Mark Straver]

(I mean of course, in the onload= in my last sentence of comment 11)

As an aside, this does not seem to trigger the crash reporter properly (it will complain about the application not properly identifying itself when it actually crashes on an OOM in mozalloc) so there likely won't be any crash reports -- that is, if people are patient enough to wait the minutes it takes to fill up the memory and not kill the task already.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Mark, do you mind filing a separate bug on that case and ccing me?  It's not really the same thing as the original (inline stylesheet) situation this bug was filed on, and it needs a different (but simpler, I think) solution.

Comment 14

[Mark Straver]

:bz - filed bug 1203973

Comment 15

[Boris Zbarsky [:bzbarsky]]

So looking at the original bug report here, it looks like that scenario was fixed in bug 748692.

We could probably declare this fixed, though there's still an issue where if onload keeps toggling between two "media" values or something we'll get a hang...

Comment 16

[Andrei Purice]

Following the reporter's steps I am able to confirm that the issues doesn't happen anymore on Windows 10 and MacOs 10.15 on any of the current versions of Firefox Nightly 87.0a1 (2021-02-16), beta 86.0 and release 85.0.2. The example test case doesn't hang Firefox anymore.

Closing this issue as Resolved > Worksforme.
Feel free to re-open or file a new bug if this issue reoccurs again.