Add an assertion to limit XBL bindings in the content process

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: bgrins, Assigned: timdream)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [xbl-in-content])

Attachments

(5 attachments)

(Reporter)

Description

a year ago
I'd like to have a way to build firefox with XBL disabled in the content process so that we can:

1) Track progress on test bustage as we get rid of bindings that use this feature
2) Measure potential perf benefits we can get once we don't need the feature anymore
Adding a MOZ_CRASH right around [1] should allow you to verify that none of the XBL scope machinery is being activated.

[1] https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/js/xpconnect/src/XPCWrappedNativeScope.cpp#292
Priority: -- → P2
(Reporter)

Comment 2

a year ago
(In reply to Bobby Holley (:bholley) from comment #1)
> Adding a MOZ_CRASH right around [1] should allow you to verify that none of
> the XBL scope machinery is being activated.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/js/xpconnect/src/
> XPCWrappedNativeScope.cpp#292

Thanks. That worked to cause an immediate crash on: `data:text/html,<marquee>hi</marquee>`. However, I don't get an immediate crash with `data:text/html,<div style="overflow:scroll; width: 50px; height: 50px;"><div style="width: 100px; height: 100px;"></div></div>`. It does crash then after I try to right click on the scrollbar itself which makes sense since that triggers a handler like: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/scrollbar.xml#16.

But I assume that even having the <content> injected into the document has to start up some XBL machinery - is there somewhere else to look to for that?
(Reporter)

Comment 3

a year ago
ni? for Comment 2. I'm specifically referring to this <content> https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/toolkit/content/widgets/scrollbar.xml#24 and the data URI with the overflow in Comment 2.
Flags: needinfo?(bobbyholley)
Comment hidden (mozreview-request)
So, one key point to note is that the reflectors for XBL anonymous content live in the XBL scope, but reflectors are lazily-instantiated. So in the scrollbar case, where we have anonymous content but not much else, we won't actually trigger the XBL scope machinery until script (in this case presumably the handlers) interacts with the content in some way.

If you want to prevent the bindings from being installed at all in the content process, one way would be to to replace the !MayBindToContent() check with an XRE_IsContentProcess() check at [1].

[1] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/dom/xbl/nsXBLService.cpp#793
Flags: needinfo?(bobbyholley)
Comment hidden (mozreview-request)
(Reporter)

Comment 7

a year ago
Updated the patch to not use <content> at all but rather just a <constructor>. Now when running with `data:text/html,<span></span>` and measuring in about:memory with / without the patch I can see some memory overhead of having a xbl script on the page. On the left you can see the extra js-zone and also the js-compartment is  bigger for this simple test case.

Note that this appears to only show up when a script actually runs, so in the scrollbar case it shouldn't be measurable until you actually interact with the scrollbar.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

a year ago
I've collected some data on the amount of memory a page like: `data:text/html,<span></span>` uses with and without a XBL binding attached to the span, using the patches here. Here's the steps to generate the data:

1) `./mach run --profile $(mktemp -d) "data:text/html,$(printf '<span></span>'%.0s {1..1})"`
2) Copy the number printed in stdout - i.e. "Total bytes: 331496"

Then repeat by incrementing the number of spans like `./mach run --profile $(mktemp -d) "data:text/html,$(printf '<span></span>'%.0s {1..10})"`. I did this for 1, 10, 100, 1000, and 10000 spans.

Here's the data I got on my system (osx, local build with tip of 6ffbba9ce0ef):

| # of spans | mem with xbl | mem w/out xbl | diff    |
|------------|--------------|---------------|---------|
|     1      |    327400    |     260648    | 66752   |
|     10     |    328904    |     263176    | 65728   |
|     100    |    396088    |     294088    | 102000  |
|     1000   |    982120    |     577352    | 404768  |
|     10000  |    6825992   |     3451208   | 3374784 |

There's better formatting, and a graph, here: https://docs.google.com/spreadsheets/d/1ZJhGfZ92YlFRcqe4Ngst4AEs4QvoAHYR1oNXru-v0bk/edit#gid=0.

I'm pretty sure that the numbers are right - I've spot checked the results both with and without xbl in about:memory verbose measurements and they match up.  Note that in the patch I do gMgr.minimizeMemoryUsage before collecting the number since it seemed more consistent.
(Reporter)

Comment 11

a year ago
A screenshot of about:memory from the final row in Comment 10
(Reporter)

Updated

a year ago
Whiteboard: [xbl-in-content]

Updated

a year ago
I am going to put an assertion in nsXBLService::LoadBindings and whitelist the allowed XBL bindings in the content process.

This is probably going to break remote XUL (+XBL) so I will probably need to check the pref there.

I will investigate what's needed here...
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Summary: Create a build configuration with XBL turned off in the content process → Add an assertion to limit XBL bindings in the content process
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #13)
> This does not consider remote XUL/XBL
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f0071fdc6b889caaac91aef768b10dde26979ead

This now considers them

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0427b2d68131e963b8e8927cea0324d663c90a63
Comment hidden (obsolete)
Could you review the patch? You didn't cancel the review so I don't think Phabricator will ask you for it again.
Flags: needinfo?(enndeakin)
I thought about Neil's review comment in the patch. I realized I would have to allow XBL usage in all of the AllowXULXBL() documents for the test setup to continue to work. The assertion, therefore, won't catch the production usage if the document had that flag flipped.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=494f53aa636cbda6fb1feca2c0acd6e895aac8bd

Perhaps the assertion can be limited one day once we removed remote XBL/XUL and disabled these XBL/XUL tests in e10s.
The patch in comment 19 works and it in Phabricator. Let me know what's needed.

Comment 21

7 months ago
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3eff39403c8c
Add an assertion to limit XBL bindings in the content process r=NeilDeakin

Comment 22

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3eff39403c8c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65

Updated

7 months ago
Flags: needinfo?(enndeakin)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.