Closed Bug 1383478 Opened 7 years ago Closed 7 years ago

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

Categories

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

54 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: doomtay, Assigned: bzbarsky)

References

Details

(Keywords: crash, regression)

Attachments

(3 files)

Attached file 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.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Assignee: nobody → bzbarsky
Flags: needinfo?(bzbarsky)
Summary: High amount of self-closing <object> tags cause a balloon in memory usage, followed by crash → High amount (number) of unclosed ("self-closing") <object> tags cause a balloon in memory usage, followed by crash
Attachment #8889603 - Flags: review?(kyle) → review+
Pushed by bzbarsky@mozilla.com:
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 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.
Attachment #8889603 - Flags: approval-mozilla-beta?
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.
Attachment #8889603 - Flags: approval-mozilla-esr52?
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
Attached file nesting.html
Summary: High amount (number) of unclosed ("self-closing") <object> tags cause a balloon in memory usage, followed by crash → High amount (number) of nested <object> tags cause a balloon in memory usage, followed by crash
Right, the key part is the nesting.  Not closing tags is just an easy way to nest them.
https://hg.mozilla.org/mozilla-central/rev/b0a3522efa57
https://hg.mozilla.org/mozilla-central/rev/f08aa694724e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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?
Flags: needinfo?(bzbarsky)
Attachment #8889603 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Oh, I had misread the "Version" field in bug 1332956 for the "Target" field.  No need for esr52 anything.
Flags: needinfo?(bzbarsky)
Attachment #8889603 - Flags: approval-mozilla-esr52?
(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.
Flags: qe-verify-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.