Closed Bug 1132743 Opened 9 years ago Closed 9 years ago

Implement a "fixlist" for use in bug 1107378

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 4 obsolete files)

Bug 1107378 will allow us to emulate support for certain -webkit prefixed properties / values. We only want to do this for sites on a "fixlist", to avoid breaking the web (since the emulation is probably not perfect) and to avoid implicitly encouraging sites to use prefixed values.

This bug is about implementing support for the "fixlist".

I'll file a separate bug on finalizing the contents of the fix list, but to start, we probably want it to contain the domains from bug 1107378 comment 4 (or maybe the domains where their stylesheets are served from?), as well as a fake domain used in mochitests so that this can be tested.

I'm marking this as blocking bug 1107378, because we can't ship bug 1107378 without this.
Blocks: 1132745
Attached patch wip fix (obsolete) — Splinter Review
Here's a first-pass fix here. Basically the strategy is:
 * In CSSParserImpl::InitScanner, grab the origin from the passed-in nsIPrincipal, and check it against our hardcoded list.
 * If there's a match, enable unprefixing in this parser (until the next time InitScanner is called)

MATCHING: I'm just using string-matching to check the origins right now (checking if it starts with "http" -- so that includes "https" URIs -- and ends with one of the domains from bug 1107378 comment 4.) The string-matching is a bit hacky right now & could probably be improved, but I'm wondering whether there's something even fancier I should be doing here for origin-matching.

Thoughts on alternatives:
* I'm wondering if there's a better place to hook in, to check the domain & enable/disable unprefixing.  (I was originally hoping to do the check *once* for a given document, and store a flag on the nsDocument, and then toggle this behavior in the nsCSSParser depending on that flag when we do parsing for that document. But I don't think we can do that, because parsing gets invoked from networking code as data arrives -- not from anything that knows about the nsDocument, AFAIK.)

* I'm not going for a super-optimal representation of the whitelist at this point -- just a static array of nsCStrings -- because it's pretty small & it's expected to stay that way. If that changes, we can re-evaluate & perhaps use a smarter data structure like a prefix tree or a hash set.  For now, I'm not convinced that the overhead of such a structure would actually be a net win. (I suppose a prefix tree *could* be a net win, but I don't think we have an implementation of that available, and it doesn't seem like it's worth spending time implementing one for this small list, at this point.)
Comment on attachment 8573336 [details] [diff] [review]
wip fix

bz, do you know if there's something better I should be doing for origin-matching here? (to selectively enable this unprefixing feature, feature based on the origin of a stylesheet)

In particular, it'd be nice to avoid having to call nsIPrincipal::GetOrigin every time we hit this code. I'm wondering if there's something I can do that just directly checks the origin. Maybe I should be using nsIPrincipal::Equals? (though then I need to create a nsIPrincipal for each entry in my whitelist, up front, I think.)
Attachment #8573336 - Flags: feedback?(bzbarsky)
er s/directly checks the origin/directly checks the principal/ (without having to ask it for a string representation of its origin, which I think might do string-copying though I'm not 100% sure)
Comment on attachment 8573336 [details] [diff] [review]
wip fix

Do you want to match any scheme (e.g. http vs https) and any port?  That is, is this a hostname whitelist, as opposed to an origin whitelist?

The simplest thing to do if so is to just GetURI on the principal, null means not in whitelist, then GetHost and compare it to the whitelist.

I you need the wildcarding behavior, then you could use nsIEffectiveTLDService to walk up towards the DNS root, right?

That said, I would really like us to cache the value somewhere (e.g. on the principal).  Doing it on each InitScanner() call means that this code runs any time someone sets foo.style.whatever, which doesn't seemed called for.
Attachment #8573336 - Flags: feedback?(bzbarsky) → feedback+
Thanks for the feedback!

(In reply to bz from comment #4)
> Do you want to match any scheme (e.g. http vs https) and any port?  That is,
> is this a hostname whitelist, as opposed to an origin whitelist?

I want to match both http & https, yes.

I don't think we really need to care about port differences; whatever's simplest there should be fine. (We can make it work either way; adding explicit things like foo.com:1234 to the whitelist as-needed (probably never), or just matching all ports and hoping for the best if that's easier.

If they have fallback CSS that we support, then the unprefixing service shouldn't have any effect on their rendering. If they don't have fallback CSS, then the unprefixing service *should* provide a strictly better rendering.  So, false-positive matches (unprefixing foo.com:1234 when we don't technically need to) aren't really a big deal.

> The simplest thing to do if so is to just GetURI on the principal, null
> means not in whitelist, then GetHost and compare it to the whitelist.

OK, thanks.

> I you need the wildcarding behavior, then you could use
> nsIEffectiveTLDService to walk up towards the DNS root

Handy. Yeah, we may need that, since in some cases this CSS for "foo.com" is served from s1.t5.foocdn.com, where presumably "1" and "5" are not permanently-fixed values.

> That said, I would really like us to cache the value somewhere (e.g. on the
> principal).  Doing it on each InitScanner() call means that this code runs
> any time someone sets foo.style.whatever, which doesn't seemed called for.

Agreed, I'd like to avoid that. I'll look into caching it on the principal (and computing it lazily, the first time it's queried).
Attached patch wip v2 (now in nsPrincipal) (obsolete) — Splinter Review
Here's an updated WIP, now with the logic living in nsPrincipal, and caching the result of our whitelist-match in a Maybe<bool> member-var (using Maybe<> to represent the "not yet initialized" vs. "initialized" status.)

I think now I just need to rewrite the mochitest to run from a variety of test domains.  I've included a test-domain "unprefixme.mochi.test" in the exact-match whitelist, and "unprefixbase.mochi.test" in the basedomain-whitelist (so e.g. "foo.unprexbase.mochi.test" will be opted-in to the unprefixing service, but "foo.mochi.test" will not.)  These test-domains are only checked if an additional pref is set, which I'll only be setting during testing -- so we won't have to check against these testing whitelist-entries in the real world.
Attached patch fix v1 (obsolete) — Splinter Review
Here's the fix, with the whitelisting logic put in nsPrincipal (lazily computed & cached) as discussed above.

There are two hardcoded whitelists -- one with a list of explicit (full) domains, & one with a list of domains whose subdomains are all implicitly whitelisted (for a few sites that serve their CSS from impermanent-looking CDN subdomains).

Each whitelist has, as its 0th entry, a "dummy" test-domain, to use in mochitests. I've added another pref (unset & implicitly-false by default), so that we can skip this 0th entry by default, in the real world.

Also worth mentioning: this patch splits up / extends the existing test (test_unprefixing_service.html) as follows:
 * unprefixing_service_iframe.html: contains most of the logic from the original test file for testing whether unprefixing is working properly.
 * test_unprefixing_service.html: loads that ^ file in an iframe from various whitelisted & not-whitelisted domains, and verifies (via a postMessage listener) that the iframe ends up having prefixing enabled (or disabled).
 * test_unprefixing_service_prefs.html: simpler test which just verifies that our about:config prefs *do* actually control unprefixing/whitelisting.
 * unprefixing_service_utils.js: contains code shared by the above two mochitests.
Attachment #8573336 - Attachment is obsolete: true
Attachment #8573593 - Attachment is obsolete: true
Attachment #8575089 - Flags: review?(dbaron)
Comment on attachment 8575089 [details] [diff] [review]
fix v1

>+bool
>+nsPrincipal::IsOnCSSUnprefixingWhitelist()
>+{
>+  if (mIsOnCSSUnprefixingWhitelist.isNothing()) {
>+    // Value not cached -- perform our lazy whitelist-check.
>+    // (NOTE: If our URI is mutable, we just assume it's not on the whitelist,
>+    // since our caching strategy won't work. This isn't expected to be common.)
>+    mIsOnCSSUnprefixingWhitelist.emplace(
>+      mCodebaseImmutable &&
>+      IsOnCSSUnprefixingWhitelistImpl(mCodebase));
>+  }

Note: I'm punting when mCodebaseImmutable is false here, as noted in the comment.

I don't know how likely it is that I'd have to handle that case; it doesn't seem to be needed for any of the sites on our initial whitelist, so I've just punted on that case for now. (i.e. I'm treating all principals with mutable mCodebase values as non-whitelisted, to err on the side of being fast & not-breaking-the-web.)

If there's a chance I need to worry about this case, for our whitelisted sites (despite it not seeming to matter right now), let me know & we can do something more robust here.
> I don't know how likely it is that I'd have to handle that case

The only way to get a non-immutable URI here is for either nsIURI::Clone to fail or for the URI to not implement nsIMutable or for nsIMutable::SetMutable to fail.  This can't happen with http URIs in stock Firefox, clearly, since we control all that and it works.

If some extension changes out the HTTP protocol handler and creates some other sort of URIs, things could fail here.  But I expect we don't so much care; chances are so much other stuff is broken with that extension anyway that it doesn't matter too much if the fixlist is not applied.
Great, thanks for that clarification, bz! Given that, I think we're on the same page & we shouldn't care about mutable URIs, for now at least, since this is just a best-effort feature with no strong guarantees, anyway.  If we discover that some frequently-used addon is making all URIs mutable & breaking this feature as a result, then we can reconsider and make the code in comment 8 more robust.
Comment on attachment 8575089 [details] [diff] [review]
fix v1

The comments in nsIPrincipal.idl should probably mention that the
reason it's a method on the principal is so that the principal object
can cache the result, for performance.

I'm not crazy about the %{C++ %} block in nsIPrincipal.idl.  (That's
risky if anybody has a scriptable interface derived from nsIPrincipal in
the future... which could end up being needed in a respin if we need to
avoid changing binary compatibility for some reason.)  What's the value
of having the inline implementation?  Why not just declare the method in
IDL syntax:
  [noscript,notxpcom,nostdcall] boolean isOnCSSUnprefixingWhitelist();
and drop the inline implementation on nsIPrincipal?


>+static bool
>+IsWhitelistingTestDomains()

Instead of messing around with a static variable sIsPrefCached, create a
new function (probably a static method on a class) to call
AddBoolVarCache, and call it from nsLayoutStatics::Initialize.

Then make IsWhitelistingTestDomains be "static inline" and just
returning the boolean.

You should probably make sure bz is ok with the caps changes (although
it sounds like he is) given that I'm not really a caps reviewer.


I didn't look at the tests all that closely; the logic to actually
run everything has gotten a bit complicated.  Please double-check that
it's running all the tests you expect it to (i.e., both the enabled
and disabled tests, for the various host options) by looking at the
test output.

r=dbaron with that
Attachment #8575089 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-7) from comment #12)
> I'm not crazy about the %{C++ %} block in nsIPrincipal.idl.  (That's
> risky if anybody has a scriptable interface derived from nsIPrincipal in
> the future... which could end up being needed in a respin if we need to
> avoid changing binary compatibility for some reason.)
[...]
> Why not just declare the method in
> IDL syntax:
>   [noscript,notxpcom,nostdcall] boolean isOnCSSUnprefixingWhitelist();

OK -- thanks. To be honest, I'm not clear on the difference between 
%{C++ %} vs. that IDL syntax.

> What's the value
> of having the inline implementation?  

It just lets us provide a default implementation for this method, so I don't need to add an impl in each nsIPrincipal subclass (e.g. nsExpandedPrincipal, nsNullPrincipal, nsSystemPrincipal).

I'm happy to provide a trivial "return false" impl in each of those places, though; seems like that may be cleaner from an IDL perspective.

> >+static bool
> >+IsWhitelistingTestDomains()
> 
> Instead of messing around with a static variable sIsPrefCached, create a
> new function (probably a static method on a class) to call
> AddBoolVarCache, and call it from nsLayoutStatics::Initialize.

Sounds good. (I did this to match local style -- there's another pref-backed bool in nsPrincipal that gets hooked up on the fly -- gCodeBasePrincipalSupport/gIsObservingCodeBasePrincipalSupport.   I'll file a followup to move that to the new init method that I'll be adding here.)

> I didn't look at the tests all that closely; the logic to actually
> run everything has gotten a bit complicated.  Please double-check that
> it's running all the tests you expect it to (i.e., both the enabled
> and disabled tests, for the various host options) by looking at the
> test output.

Yeah :-/ The async nature of running tests in iframes made this a bit more complex. (It could probably be made saner with promises, but I haven't taught myself to think in terms of promises yet, and I didn't want to gate this on that.)

I added the second test here (to check the prefs' effectiveness) in part as a layer of sanity-checking, BTW. And I'll do a manual sanity-check on the test output before landing, too.
Attached patch fix v2 [r=dbaron] (obsolete) — Splinter Review
bz: per dbaron's point in comment 12, I should be sure you're OK with the /caps changes here -- would you mind taking a look? (particularly at nsIPrincipal.idl and nsPrincipal.cpp.)

(You may have already taken a look in comment 10; the only changes since then are for comment 12's review comments.)
Assignee: nobody → dholbert
Attachment #8575089 - Attachment is obsolete: true
Attachment #8576997 - Flags: feedback?(bzbarsky)
OS: Linux → All
Hardware: x86_64 → All
(In reply to Daniel Holbert [:dholbert] from comment #13)
> (In reply to David Baron [:dbaron] (UTC-7) from comment #12)
> > I'm not crazy about the %{C++ %} block in nsIPrincipal.idl.  (That's
> > risky if anybody has a scriptable interface derived from nsIPrincipal in
> > the future... which could end up being needed in a respin if we need to
> > avoid changing binary compatibility for some reason.)
> [...]
> > Why not just declare the method in
> > IDL syntax:
> >   [noscript,notxpcom,nostdcall] boolean isOnCSSUnprefixingWhitelist();
> 
> OK -- thanks. To be honest, I'm not clear on the difference between 
> %{C++ %} vs. that IDL syntax.

With %{C++ %}, other uses of the IDL (via the XPT typelibs and xptcall and either XPConnect or XPCOM proxies) won't know about the method; this means if there are later methods or derived interfaces, they'll get confused about vtable offsets.
Ah, ok. And I guess the already-existing %{C++ %} chunks in nsIPrincipal.idl don't have that problem because they're non-virtual, so they won't impact the vtable.  Makes sense -- thanks for the clarification.
Comment on attachment 8576997 [details] [diff] [review]
fix v2 [r=dbaron]

>+// XXXdholbert Add to InitializeStatics():

Done, no?

>+  static const nsCString sFullDomainsOnWhitelist[] = {

Will this not leak to shutdown leak logging failing?  I guess with literal strings it might not, since they don't allocate stringbuffers... In any case, watch out for that when landing.

>+    // normal codepath, for the benfit of a disabled test domain.)  If we add 

s/benfit/benefit/

>+    for (size_t subdomainDepth = 0;

subdomainDepth should probably be uint32_t, since that's the type of the argument it gets passed to.

>+  // for an expanded principal. (and probably shouldn't be needed)

"And"

The caps bits look fine modulo the above nits.
Attachment #8576997 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Not doing reviews right now from comment #17)
> >+// XXXdholbert Add to InitializeStatics():
> 
> Done, no?

Not quite -- that comment is for gCodeBasePrincipalSupport (a pre-existing variable, unrelated to this bug, which is currently hooked up lazily via AddBoolVarCache, but we should now hook it up it non-lazily, since we've got a static method for just this sort of thing.)

I'm planning to fix that separately, so that this bug's patch doesn't mess with unrelated code.

(I'll probably just do that as a trivial followup here; just wanting to keep the main patch targeted.)

> >+  static const nsCString sFullDomainsOnWhitelist[] = {
> 
> Will this not leak to shutdown leak logging failing?  I guess with literal
> strings it might not, since they don't allocate stringbuffers... In any
> case, watch out for that when landing.

We have several instances of this pattern in the codebase already, for non-C-strings -- static arrays of NS_LITERAL_STRING -- so I expect it should be fine. I'm now noticing that we generally declare them as nsLiteralString, so I probably use nsLiteralCString as the type here.  I'll make that change.

I fixed the typos & s/size_t/uint32_t/ things that you caught, too -- thanks for the feedback!
Updated fix w/ bz's review feedback addressed; will land tomorrow at some point.
Attachment #8576997 - Attachment is obsolete: true
(Here's the followup patch to address the XXXdholbert comment noted above. I'm not bothering with review on this since it's trivial, and it's in the spirit of dbaron's request in second half of comment 12 -- hence I'm using "implicit rs=dbaron" in commit message.)
Blocks: 1143147
(s/failures/timeouts/, in test_cache_matchAll_request.html and test_cache_match_request.html )

Looks like these tests normally take 5-10 seconds on this platform, but in that log with the timeouts, they both took > 5 minutes (hence, timeout).

There were some retriggers pending for that job, though, which have now completed, and the retriggers are in the normal 5-10 second duration, e.g.:
15:17:43     INFO -  1187 INFO TEST-START | dom/cache/test/mochitest/test_cache_matchAll_request.html
[...]
15:17:51     INFO -  1188 INFO TEST-OK | dom/cache/test/mochitest/test_cache_matchAll_request.html | took 8210ms
15:17:51     INFO -  1189 INFO TEST-START | dom/cache/test/mochitest/test_cache_match_request.html
[...]
15:17:56     INFO -  1190 INFO TEST-OK | dom/cache/test/mochitest/test_cache_match_request.html | took 5086ms
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-asan/1426277835/mozilla-inbound_ubuntu64-asan_vm_test-mochitest-e10s-1-bm67-tests1-linux64-build3.txt.gz

Given that new data, I want to claim that the timeout was sporadic, and that suspicion is now lowered for my specific push.

I'll wait a bit before relanding, so we can see if this happens again & to allow time for more retriggers to complete.
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/3c7cdd745877
https://hg.mozilla.org/mozilla-central/rev/14b7ae148c8f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(BTW: I noticed one last reference to this bug in a comment, and pushed a commit to remove it:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/955d0073cf9a )
See Also: → 1177263
You need to log in before you can comment on or make changes to this bug.