Closed Bug 1426492 Opened 6 years ago Closed 6 years ago

Add an assertion to limit XBL bindings in the content process

Categories

(Core :: DOM: Core & HTML, task, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: bgrins, Assigned: timdream)

References

Details

(Whiteboard: [xbl-in-content])

Attachments

(5 files)

Bug 1426492 - Test case with trivial binding attached to the span element
59 bytes, text/x-review-board-request
Details
about-memory-with-and-without-xbl.png
219.88 KB, image/png
Details
Bug 1426492 - Print out memory used by data url for testing with/without xbl
59 bytes, text/x-review-board-request
Details
about-memory-10000-spans.png
384.88 KB, image/png
Details
Bug 1426492 - Add an assertion to limit XBL bindings in the content process
46 bytes, text/x-phabricator-request
Details | Review 
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
(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?
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)
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)
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.
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.
A screenshot of about:memory from the final row in Comment 10
Whiteboard: [xbl-in-content]
Blocks: war-on-xbl-in-content
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
Moves the lines down a bit so we don't assert on bindings that won't be loaded.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=533f8d0ce6b34cfd59f3962eccfd9d1fe4f636ab
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.
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
https://hg.mozilla.org/mozilla-central/rev/3eff39403c8c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: needinfo?(enndeakin)
Component: DOM → DOM: Core & HTML
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: