Closed Bug 1203973 Opened 4 years ago Closed 4 years ago

Changing media in onload= in <link> hangs Firefox


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox41 --- ?
firefox42 --- verified
firefox43 --- verified
firefox44 --- verified


(Reporter: mark, Assigned: bzbarsky)




(2 files)

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 has made a recent change that triggers a hang leading to an OOM in Firefox.

Offending code:

        <link rel="stylesheet" href="" media="none" onload="media='all'" />
        <link rel="stylesheet" href="" 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 ;).
See Also: → 693725
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).
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....
Attachment #8664601 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Ever confirmed: true
Try run is at so at last we don't have any tests checking for that behavior...
(We should make SetAttr in Element final or something, and force everyone to use Before/AfterSetAttr)
but href sets don't seem to do reloading, at least when tested locally.
Trying to understand what comment 2 is about.
I guess you mean dropSheet = !(linkTypes & nsStyleLinkElement::eSTYLESHEET); case when that is true.
er, no, that is about rel
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 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?
Attachment #8664601 - Flags: review?(bugs) → review+
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 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.  :(
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.
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
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?
Flags: needinfo?(bugs)
given that comment 2 wasn't an issue after all, this looks like rather safe fix to me.
Flags: needinfo?(bugs)
So definitely Aurora, and we're early in cycle so perhaps beta too
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.
Attachment #8664601 - Flags: approval-mozilla-beta?
Attachment #8664601 - Flags: approval-mozilla-aurora?
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?
Attachment #8664601 - Flags: approval-mozilla-beta?
Attachment #8664601 - Flags: approval-mozilla-beta+
Attachment #8664601 - Flags: approval-mozilla-aurora?
Attachment #8664601 - Flags: approval-mozilla-aurora+
> 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).
Flags: qe-verify+
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).
Flags: qe-verify+
Duplicate of this bug: 1252090
You need to log in before you can comment on or make changes to this bug.