905441 - Load style sheets from the parent into the child

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: style-sheets

This is needed for adblock, which uses a big style sheet to blacklist certain kinds of images and elements.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Seems like it would be better for this code to be entirely in the back-end (nsStyleSheetService) if possible.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 790517 [details] [diff] [review]
style-sheets

I talked to Gavin and he's convinced me that we should do this somewhere in the style sheet service code instead of in JS. I'm thinking we would forward new style sheets to the child using a new message in the PContent protocol.

Tom, is there any chance you could take a look at this? If not, I'll try to get to it on Monday.

Comment 3

[Tom S [:evilpie]]

I can do this. I already have the forwarding for LoadAndRegisterSheet and UnregisterSheet done. The next thing is requesting all the stylesheets from the parent, when we nsIStyleSheetService instance in the child.

Comment 4

[Tom S [:evilpie]]

Attachment: v1

So this implements everything in C++. When somebody calls loadAndRegisterSheet or unregisterSheet in the parent we asynchronously send that to all children as well. The other case is when a new child is created it asks for all style-sheets in the parent, this has a small flaw, because we also send those in the category manager. But actually checking if something is already in that manager is a bit more work. We could potentially just never use the category manager in content.

Comment 5

[Tom S [:evilpie]]

Comment on attachment 791477 [details] [diff] [review]
v1

Roc for the layout bits and smaug for content? I can't really tell who is a good reviewer here.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 791477 [details] [diff] [review]
v1

dbaron should definitely review this.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I'm not crazy about the "doing both" aspect of comment 4 -- i.e., having the child both get the sheets from the parent or from the category manger.  I'm inclined to think we ought to be doing one or the other, though I'm also not all that familiar with the architecture here (i.e., how parents and children ought to interact in general, how setup/teardown works).

Having the GetStyleSheets be sync also seems a little disturbing.  (Sync certainly feels bad to me, though I'm not sure how worried I should actually be.)  Could the parent instead send the sheets async to the child during the process of setting up the child?

I'm curious what others think fits better with the model of how we set up child processes.  needinfo? to jlebar for that since he seems like somebody who might know that, but feel free to pass along to somebody else if more appropriate.

Comment 8

[Justin Lebar (not reading bugmail)]

> Could the parent instead send the sheets async to the child during the process of setting up the 
> child?

That seems pretty sane.  Then we'd only need one mechanism for getting stylesheets from the parent to the child, and we'd avoid a sync call.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 791477 [details] [diff] [review]
v1

Review of attachment 791477 [details] [diff] [review]:
-----------------------------------------------------------------

This is all dbaron :-)

Comment 10

[Tom S [:evilpie]]

Okay I will look at where is a good point to send the stylesheets to the child.

Comment 11

[Tom S [:evilpie]]

Attachment: v2

In this new patch we send the load during the ContentChild creation.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I was just discussing this issue with Fabrice; two other issues we discussed was:

 (a) whether userContent.css is currently broken in child processes in B2G (since child processes can't access the profile), and

 (b) that B2G probably has a theme-ing use case for loading style sheets via the style sheet service (and app: URLs) that would fail security checks in other child processes.

That perhaps suggests we'd eventually want this to be sending the style sheet rather than its URL.  That said, that issue probably ought to go in a different bug; I was thinking it was closely related to this one, but it should probably be distinct.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 792889 [details] [diff] [review]
v2

And I'm going to transfer this review request to bz; he knows this code better.  (Yes, both of our review queues are overloaded, I think, but...)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 792889 [details] [diff] [review]
v2

>+    SerializeURI(aSheetURI, uri);
>+
>+    nsTArray<dom::ContentParent*> children;
>+    dom::ContentParent::GetAll(children);

Is the idea that children will end up empty in child processes?

But why pay the SerializeURI cost in that case?  Can we condition this on the process type?

r=me modulo that

Comment 15

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7be13ec66043

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7be13ec66043