1383478 - High amount (number) of nested <object> tags cause a balloon in memory usage, followed by crash

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[doomtay]

Attachment: crashtest.html

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Open a webpage with a high amount of self-closing <object> tags like http://vesta.janusvr.com/ or http://www.janusvr.com/newlobby/index.html or even the attached file
Wait


Actual results:

Growing increase in memory consumption followed by crash


Expected results:

The page loads normally, even if in the last two cases, it's blank.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=52f3dec57461b9d7b958779a67cea4187f820a81&tochange=ca298ab420e09a5f7a643c6bcfa9bde539b0573c

Regressed by: Bug 1332956

Comment 2

[Boris Zbarsky [:bzbarsky]]

doomtay, thank you for the excellent bug report and clear testcase!

So the key part here is that there are a bunch of unclosed object tags here.

The logic triggered in nsObjectLoadingContent by bug 1332956 does this: when an <object> falls back, it starts loads in all its descendant <object> or <embed> elements.

In this case, we have 40 nested <object>s.  The first one fails to load, starts loads in the remaining 39.  The second one fails to load, starts loads in the remaining 38.  The third fails to load, starts loads in the remaining 37, etc.  All of this is actually sync, by the way: LoadFallback synchronously calls StartObjectLoad which synchronously calls LoadObject, which synchronously calls LoadFallback on the descendants.

So we get one load "started" in the second <object>, two in the third one, 4 in the fourth.  The 40th would get 2^39, which is quite a large number, of course.

I'm not sure what the memory allocations are that cause memory to balloon, but if each of those load attempts queues a runnable _somewhere_, that sure would do it.

Anyway, fix coming up.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure <object>/<embed> fallback doesn't try to trigger loads on <object>/<embed> descendants that aren't directly affected by whether we loaded

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0a3522efa57
Make sure <object>/<embed> fallback doesn't try to trigger loads on <object>/<embed> descendants that aren't directly affected by whether we loaded.  r=qdot

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f08aa694724e
Add tests for bug 1383478

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8889603 [details] [diff] [review]
Make sure <object>/<embed> fallback doesn't try to trigger loads on <object>/<embed> descendants that aren't directly affected by whether we loaded

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1332956
[User impact if declined]: A denial of service crash that can be triggered
   pretty easily by accident.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not very.
[Why is the change risky/not risky?]: As anything dealing with object/embed
   there is some risk.  But the new code is clearly more correct than the old,
   and assuming other bits of the code work as they should (which is an
   assumption, I know) should not cause any problems.
[String changes made/needed]: None.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8889603 [details] [diff] [review]
Make sure <object>/<embed> fallback doesn't try to trigger loads on <object>/<embed> descendants that aren't directly affected by whether we loaded

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Easy to trigger crash, but a safe one.
User impact if declined: See comment 6.
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): See comment 6.  The other
   option is to do nothing on ESR.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 8

[doomtay]

I've noticed that there's crashing even if the <object> tags are closed, but nested. I'll try and add a demonstration of this

Comment 9

[doomtay]

Attachment: nesting.html

Comment 10

[Boris Zbarsky [:bzbarsky]]

Right, the key part is the nesting.  Not closing tags is just an easy way to nest them.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b0a3522efa57
https://hg.mozilla.org/mozilla-central/rev/f08aa694724e

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8889603 [details] [diff] [review]
Make sure <object>/<embed> fallback doesn't try to trigger loads on <object>/<embed> descendants that aren't directly affected by whether we loaded

ok let's get this one into 55.0b13.

Boris, I'm unclear on the esr52 status.  Comment 6 says this was introduced by bug 1332956 which is only in 54, so esr52 should be unaffected?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, I had misread the "Version" field in bug 1332956 for the "Target" field.  No need for esr52 anything.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/07dc44f88f12
https://hg.mozilla.org/releases/mozilla-beta/rev/d1342d04957e

Comment 15

[Andrei Vaida [:avaida]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Not yet.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Boris's assessment on manual testing needs and the fact that this fix has automated coverage.