1203973 - Changing media in onload= in <link> hangs Firefox

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mark Straver]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.7) Gecko/20150824 Firefox/31.9 PaleMoon/25.7.0
Build ID: 20150824130139

Steps to reproduce:

Looks like http://whatsmyuseragent.com has made a recent change that triggers a hang leading to an OOM in Firefox.

Offending code:

        <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'" />

Any css will do, even empty, the only thing we need is the "onload" event fired. it's enough to assign any string to `media` (and only `media`) there to cause hangup.




Actual results:

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 onload= in a stylesheet <link> will cause DoS, AFAICT.

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.


Expected results:

The page should have loaded (with lazy-loaded stylesheet) without a hang or crash (obviously ;).

Comment 1

[Mark Straver]

Attachment: Test case (hangs Firefox)

Added a minimal test case here that demonstrates the issue (the owner of the site has responded and updated, solving the issue on the live site).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Move <style> and <link> attribute change handling to AfterSetAttr

Olli, one thing that worries me here is whether we need to keep href sets reloading the sheet even if they're no-ops otherwise....

Comment 3

[Boris Zbarsky [:bzbarsky]]

Try run is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b426a4abde4a so at last we don't have any tests checking for that behavior...

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(We should make SetAttr in Element final or something, and force everyone to use Before/AfterSetAttr)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

but href sets don't seem to do reloading, at least when tested locally.
Trying to understand what comment 2 is about.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I guess you mean dropSheet = !(linkTypes & nsStyleLinkElement::eSTYLESHEET); case when that is true.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

er, no, that is about rel

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

We do have 
if (!aForceUpdate && mStyleSheet && !isInline && uri) {
...
nsresult rv = oldURI->Equals(uri, &equal);
if (NS_SUCCEEDED(rv) && equal) {
  return NS_OK; // We already loaded this stylesheet
}

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8664601 [details] [diff] [review]
Move <style> and <link> attribute change handling to AfterSetAttr

r+, but please explain what comment 2 is about.
What am I missing here?

Comment 10

[Mark Straver]

Local testing with a ported patch shows this solves the issue wit the test case I included.

As an aside: I didn't set the importance on this bug; I gather it should be critical or maybe even a blocker, considering the result if unpatched (and Google's page speed insights apparently suggesting the change to <link> that tickled this bug and caused the initial report). Might be a good idea to uplift this.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment 2 was about me not having carefully read the code to see what the current behavior is...  And you're right that the current behavior is to not reload the sheet on href sets if the href doesn't change. So we're all good here, and my apologies for making you look through that stuff.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Mark, uplifting this is a bit scary; this is not a risk-free change.

I would probably be OK with uplifting this to Aurora.  Not sure about Beta, much less ESR.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37e3bc54a403

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37e3bc54a403

Comment 15

[Mark Straver]

Boris, I think you're taking a bigger risk by leaving this sit another 3 cycles because webmasters following Google's webspeed advisories will hang Firefox with the suggested improvement of lazy loading.

(In reply to Boris Zbarsky [:bz] from comment #12)
> Mark, uplifting this is a bit scary; this is not a risk-free change.

I'm curious: what exactly is the risk here? It doesn't seem to me like anything particularly scary is being done by moving the handling to a later point in time.

Comment 16

[Boris Zbarsky [:bzbarsky]]

It's not just a later point in time.  The thing that fixes the testcase in question is that AfterSetAttr doesn't run in various cases in which SetAttr runs, and in particular it does not run if the new value is the same as the old value.  The risk is that there is a case we didn't consider in which we would now not run the code but used to and should.

Olli, how comfortable are you with uplifting this stuff?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

given that comment 2 wasn't an issue after all, this looks like rather safe fix to me.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

So definitely Aurora, and we're early in cycle so perhaps beta too

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8664601 [details] [diff] [review]
Move <style> and <link> attribute change handling to AfterSetAttr

Alright, let's try this.

Approval Request Comment
[Feature/regressing bug #]: None; bigger problem due to website changes
[User impact if declined]: Browser hangs on some websites using coding
   techniques Google is advocating.
[Describe test coverage new/current, TreeHerder]: Probably so-so...
[Risks and why]: Risk is probably low, but not negligible.
[String/UUID change made/needed]: None.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8664601 [details] [diff] [review]
Move <style> and <link> attribute change handling to AfterSetAttr

OK, let's try with this patch. Should be in 42 beta 3.

Any reason why the test didn't land too?

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5da94c05c7a9

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5da94c05c7a9

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab880d565bb0

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Any reason why the test didn't land too?

Yes, I couldn't quite figure out how to actually test this in our harnesses (and in particular how to ensure that the test runs long enough that it would catch the issue if it were present).

Comment 25

[Bogdan Maris, Desktop QA]

Reproduced the hang on old Nightly (2015-09-27), verified that Firefox 42 beta 4, latest Aurora 43.0a2 and latest Nightly 44.0a1 will not hang across platforms (Windows 7 64-bit, Windows 10 64-bit, Mac OS X 10.10.5 and Ubuntu 12.04 32-bit).