Closed Bug 1046166 Opened 9 years ago Closed 6 years ago

[e10s] userContent.css does not work

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86_64
Windows 7
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
e10s + ---
firefox51 - wontfix
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: alice0775, Assigned: wcpan)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: e10st?)

Attachments

(4 files, 1 obsolete file)

Bug 1046166 - Send userContent.css URL to content processes.
58 bytes, text/x-review-board-request
dbaron
: review+
jcristau
: approval-mozilla-aurora+
Details
Bug 1046166 - Add sandbox white list for userContent.css on MacOSX.
58 bytes, text/x-review-board-request
haik
: review+
Details
Bug 1046166 - Add sandbox white list for userContent.css on Windows.
58 bytes, text/x-review-board-request
bobowen
: review+
Details
0001-Bug-1046166-Send-userContent.css-URL-to-content-proc.patch
7.24 KB, patch
Details | Diff | Splinter Review 
https://hg.mozilla.org/mozilla-central/rev/f61a27b00e05
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140730030201
Alice, is this userContent.css problem a regression from the changeset you link in comment 0? Is this really an IPC bug?
Blocks: fxe10s
tracking-e10s: --- → ?
Flags: needinfo?(alice0775)
Keywords: regression
(In reply to Chris Peterson (:cpeterson) from comment #1)
> Alice, is this userContent.css problem a regression from the changeset you
> link in comment 0? Is this really an IPC bug?

Sorry, I am not sure which component is best.
At least, the problem exists since Firefox25's e10s.
Flags: needinfo?(alice0775)
Assignee: nobody → wmccloskey
Assignee: wmccloskey → nobody
Component: IPC → CSS Parsing and Computation
Hi.

As a Nightly user, I would like to help test e10s, but I am also a heavy user of userContent.css (rather than, say, Stylish, because it's baked into Firefox and dead simple to use), so I always end up reverting `browser.tabs.remote.autostart` each time e10s is pushed to me.

Also, userContent.css / userChrome.css look very much like the kind of [advanced + little-discoverable] feature that Mozilla might want to sever to reduce the configurable surface and encourage usage of an extension. This said (and sorry for the "ME TOO, and by the way I have a question" digression), is there a possibility this bug ends up WONTFIXed, or am I dreaming this up and it's unfixed simply because nobody had the time to look at it yet?
browser.tabs.remote.autostart.1 was set to true in today's Nightly 38.0a1 (2015-01-14), and I re-flipped it to false to have my userContent.css working.

Is the feature going to be supported at all in e10s?
That's the plan, it's just a much lower priority than all of the other work happening.
Version: 34 Branch → Trunk
The Nightly and Aurora now

Directory "chrome" important for many people!

Me, forced to disable multi-process !
Hello
With the update of Firefox Developer Edition 16-07-2015 42.0a2, chrome folder that contains userContent.css no longer works ...

To solve this regression, we must unfortunately disable multi-process mode for userContent.css is again considered.
If somebody wants to debug this, a good place to start would be nsLayoutStylesheetCache::InitFromProfile (in the content process).  My guess would be that something it does doesn't work in the content process.

https://hg.mozilla.org/mozilla-central/file/0876695d1abd/layout/style/nsLayoutStylesheetCache.cpp#l397
I think what's happening is that we don't know the location of the profile directory in the content process. That's to avoid accessing the file system, which will be forbidden once we have a full sandbox. I'm guessing we're returning here:
https://hg.mozilla.org/mozilla-central/file/0876695d1abd/layout/style/nsLayoutStylesheetCache.cpp#l414

I think we need to design something here that will send down the style data from the parent. Maybe we could load the style sheet text in the parent and send it down to each child upon startup. Does that seem reasonable David?
(In reply to Bill McCloskey (:billm) from comment #9)
> I think what's happening is that we don't know the location of the profile
> directory in the content process. That's to avoid accessing the file system,
> which will be forbidden once we have a full sandbox. I'm guessing we're
> returning here:
> https://hg.mozilla.org/mozilla-central/file/0876695d1abd/layout/style/nsLayoutStylesheetCache.cpp#l414

Hi, I'm working on this bug (observers: no promises made! I'm not an employee and this is my first tentative at mozilla dev; just giving it a try, maybe I'll succeed and maybe not).

I didn't validate this and thought the child didn't have to pass through this code. With Bill's suggestion and now understanding a bit more about e10s, I'm going to validate this with logging. Logging is my only option for this, right? Reading the e10s/debugging wiki page [1] and mconley's blog post [2], I don't see a way to tell gdb to "attach to any child at nsLayoutStylesheetCache.cpp:l414" in order to catch such early initialization work. Wrong?

> I think we need to design something here that will send down the style data
> from the parent. Maybe we could load the style sheet text in the parent and
> send it down to each child upon startup. Does that seem reasonable David?

Also, how should non-e10s windows be covered? 
 a. Branch to keep using to the current code if in a non-e10s window.
 b. Do the kind of message-passing you describe in non-e10s windows too.

[1] https://wiki.mozilla.org/Electrolysis/Debugging
[2] http://mikeconley.ca/blog/2014/04/25/electrolysis-debugging-child-processes-of-content-for-make-benefit-glorious-browser-of-firefox/
Bill is probably a better person to answer comment 10 than I am, although I could try...
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #9)
> I think we need to design something here that will send down the style data
> from the parent. Maybe we could load the style sheet text in the parent and
> send it down to each child upon startup. Does that seem reasonable David?

I guess it does... although maybe we could instead piggy-back on the way we load the UA style sheets, which is with resource:// URLs.  Then again, maybe it's problematic that the resource: URLs for the UA sheets are (I think) in JARs, and these wouldn't be.
We actually have some existing code to send style sheet loads down to the child process:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2570
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsStyleSheetService.cpp#160

David, is there a way we could use this code for userContent.css. I'm not familiar with all the loading mechanisms at play here.
Flags: needinfo?(wmccloskey) → needinfo?(dbaron)
I don't think that helps with this (at least in the long term in terms of security models as you describe in comment 9), since that just sends a URI down to the content process, which it then invokes network code on, which goes back to the parent process to do the load.  Whereas here the content process already knows it needs to load a particular URI, it just needs a way to tell the parent process to load that URI in a way that the parent process can tell is safe.  I suppose that we could have the parent process get the profile directory, but it sounds like we don't want the child process to know where the profile directory is or have access to everything in it, so it seems better to have a special URI just for these two style sheets (userContent.css and userChrome.css).
Flags: needinfo?(dbaron)
Flags: needinfo?(wmccloskey)
Maybe crazy, but could we just use a data: URL? It would be a lot easier. I doubt users are using gigantic style sheets here.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #15)
> Maybe crazy, but could we just use a data: URL? It would be a lot easier. I
> doubt users are using gigantic style sheets here.

Hi. No idea how representative I am of the users of this feature, just providing one data point: after a few years of usage, my userContent.css has rules for ~30 web sites, and is 10kB.
Hi. Chiming in to add two things:

1. (Addressed to other users of the feature) Extension "dotjs" [1] is a 100%-"compatible" alternative to userContent.css. Just symlink your userContent.css to ~/.css/default.css and you're done. There are other ways to use the extension (e.g. one file per domain), but since it's fine with the `@-moz-document...` rules our userContent.css uses and supports this `default.css` special case that is loaded on all domains, it's feature-identical (and more) to userContent.css . One regression is that the css isn't instantly applied, on my machine there is a maybe ~500ms on load without the custom style, then it's applied.

2. (Addressed to potential developers of this bug) Actually, the extension is *better* than userContent.css, because changes to the custom css file don't require a Firefox restart, only a page reload. If work is done to restore the feature in e10s, it would be nice to not require a restart too. Or better, it would be *super* nice to monitor the css file and apply changes instantly (like Chrome's `User Stylesheet.css` used to do before the feature was axed)


[1] https://github.com/rlr/dotjs-addon , https://addons.mozilla.org/en-US/firefox/addon/dotjs/
https://addons.mozilla.org/en-US/firefox/addon/dotjs/ is not signed

Even if so it never be look truster then core userContens.css Firefox feature...
See Also: → 1187099
userChrome.css and userContent.css not work also on Firefox Aurora Developer Edition 46.0a2. Problem with add-on uc https://addons.mozilla.org/pl/firefox/addon/uc/
-> tracy for a qa report.
tracking-e10s: + → ?
Flags: needinfo?(twalker)
I put little style change in /Users/tracy/mozilla-central/obj-x86_64-apple-darwin13.3.0/dist/bin/browser/defaults/profile/chrome/useContent.css

/* Put a thin black border around all dropdown forms like NS4.x */
:-moz-dropdown-list {
  border: 5px solid black !important;
  border-top-style: solid !important;
}

I am not sure why/how, but this change is affecting all of my Firefox/Nightly installations. Anyway, the css works in non-e10s but fails to draw a small black border around form drop downs in e10s mode.

I tested with latest Nightly 47.0a1, 20160203030249 on Mac 10.9.5.  I can't figure out how to replicate on Windows or Linux, short of building Nightly, which I am not setup to do on those platforms.  

Also note:  I tried using Stylish and one of its basic/popular style changes and it does work in e10s.
Flags: needinfo?(twalker)
Kev: I think we should drop support for userContent.css and suggest that people replace it with greasemonkey-like addons (this can be done using the addon SDK as well as webextensions). Can you confirm that decision and if so we'll morph this bug into removing userContent.css support in general in favor of an addon-based approach.
Flags: needinfo?(kev)
userchrome.css and usercontent.css are have more features. Greasemonkey is for web/javascript, but Stylish is for change web style and Firefox theme/style
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #23)
> Kev: I think we should drop support for userContent.css and suggest that
> people replace it with greasemonkey-like addons (this can be done using the
> addon SDK as well as webextensions). Can you confirm that decision and if so
> we'll morph this bug into removing userContent.css support in general in
> favor of an addon-based approach.

Maybe you could add "user script addons" instead of a third party addon for user scripts. Something that mostly just encapsulates a script and some metadata, that can be easily edited with a built in editor. (Which already exists: Scratchpad)
I tested add-on UC and userchrome.css work great  
uc version 20160205 
https://addons.mozilla.org/pl/firefox/addon/uc/

More about this addon:
Allows you to run arbitrary custom scripts in specified chrome windows.
In short, userChromeJS + subscriptoverlayloader.js.
usercontent.css is for only web. Stylish is more powerful
I would like to say that while I am not currently using e10s, I do rely on userContent.css for in my opinion a rather critical part of my user experience that should not need an extension. I use a dark GTK theme, and one of the first things I noticed were the issues noted here: https://wiki.archlinux.org/index.php/firefox#Unreadable_input_fields_with_dark_GTK.2B_themes. While I do use Stylish for other things, I think that I should not need a browser extension/add-on in order to have a non-broken user experience when using a dark GTK theme.
I don't know this is related to Bug 1221724 - Remove xulrunner from mozilla-central, userContent.css seems to work in the latest central with e10s.
Can anyone confirm?
> I don't know this is related to Bug 1221724 - Remove xulrunner from
> mozilla-central, userContent.css seems to work in the latest central with
> e10s.

userContent.css still does not work in the latest central.
I've rushed for a conclusion.
I'm sorry about my fault.
Jeff, any opinion here? Tjis bug is currently caught in triage limbo waiting on feedback from Kev.
Flags: needinfo?(jgriffiths)
(In reply to Jim Mathies [:jimm] from comment #46)
> Jeff, any opinion here? Tjis bug is currently caught in triage limbo waiting
> on feedback from Kev.

I think we should fix this instead of waiting for an add-ons plan that will require migration. This is a regression and there seems to be some ideas in the previous comments on how to fix it.
tracking-e10s: ? → +
Flags: needinfo?(jgriffiths)
Priority: -- → P1
Adding that using the .dotjs extension [1] is not an ideal replacement. I'm trying it but it reads the CSS files after render causing pages to display the original and then jump to update the desired styles.

userContent.css would do this at a different execution time and cause the page to render as desired initially.

[1] https://addons.mozilla.org/en-US/firefox/addon/dotjs/
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #23)
> Kev: I think we should drop support for userContent.css and suggest that
> people replace it with greasemonkey-like addons (this can be done using the
> addon SDK as well as webextensions).

Fixing a feature that is not working due to e10s by using an addon that may again not work due to e10s (or will get broken by any other change in the future)?
Flags: needinfo?(kev)
Whiteboard: e10st?
Will XBL (or more generally external resources) in userContent work with this patch? (See bug 1232903)
Flags: needinfo?(wpan)
Oh, didn't know that we support that!
But don't worry, this patch works fine with XBL resources.
Flags: needinfo?(wpan)
https://reviewboard.mozilla.org/r/66392/#review63886

One comment so far; still need to look at this more:

::: layout/style/nsLayoutStylesheetCache.cpp:344
(Diff revision 1)
>    LoadSheetURL("chrome://global/content/xul.css",
>                 &mXULSheet, eAgentSheetFeatures);
>  
> +  if (gUserContentSheetURL) {
> +    LoadSheet(gUserContentSheetURL, &mUserContentSheet, eUserSheetFeatures);
> +    gUserContentSheetURL = nullptr;

nsLayoutStylesheetCache::Shutdown() should probably assert that gUserContentSheetURL is null.
Assignee: nobody → wpan
https://reviewboard.mozilla.org/r/66392/#review66358

::: layout/style/nsLayoutStylesheetCache.cpp:343
(Diff revision 1)
>                 &mSVGSheet, eAgentSheetFeatures);
>    LoadSheetURL("chrome://global/content/xul.css",
>                 &mXULSheet, eAgentSheetFeatures);
>  
> +  if (gUserContentSheetURL) {
> +    LoadSheet(gUserContentSheetURL, &mUserContentSheet, eUserSheetFeatures);

What type of url is this? Is it file://?
(In reply to Jim Mathies [:jimm] from comment #56)
> https://reviewboard.mozilla.org/r/66392/#review66358
> 
> What type of url is this? Is it file://?

Yes, it starts with file:// .
Hitting this now on FF 48 with "browser.tabs.remote.autostart" set.  Given:

https://wiki.archlinux.org/index.php/firefox#Unreadable_input_fields_with_dark_GTK.2B_themes

Then I need userContent.css, as someone mentioned in prior comment.  Well, the dark themes thing is actually a bad doing on FF side as well (and userContent.css is used to work it around), but seems not getting enough attentio...  I'm counting on getting this one fixed, :-)
:dbaron, should I choice another peer to reduce your review queue?
If so, which peer should I choose instead?

Thank you.
Flags: needinfo?(dbaron)
Comment on attachment 8773650 [details]
Bug 1046166 - Send userContent.css URL to content processes.

https://reviewboard.mozilla.org/r/66392/#review68518

OK, r=dbaron with that comment (above) addressed.

I'm not sure if this will keep working in the long term, but it will probably be ok for a bit.
Attachment #8773650 - Flags: review?(dbaron) → review+
Sorry for the delay getting to this; done now.
Flags: needinfo?(dbaron)
Keywords: checkin-needed
Please update the review request to resolve comment #54.
Keywords: checkin-needed
Fixed issue in comment 54, also added some assert to ensure that parent process wont set gUserContentSheetURL.

Test result on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c66a2156fd


(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #61)
> Sorry for the delay getting to this; done now.

Thanks!
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d1d614497fbf
Send userContent.css URL to content processes. r=dbaron
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d1d614497fbf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Can there be any attempt to port the change to FF 48?  As indicated on comment [58]?  FF 48 already exposes it...
(In reply to Javier from comment #67)
> Can there be any attempt to port the change to FF 48?  As indicated on
> comment [58]?  FF 48 already exposes it...

If this patch does not cause further regression then I think at least we can uplift this to 50.

:dbaron, do you think we can uplift to 49 or 48?
Flags: needinfo?(dbaron)
After landing hg.mozilla.org/mozilla-central/, latest Central (20160817030202 https://hg.mozilla.org/mozilla-central/rev/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74) crashes with e10s enabled when using userContent.css.

1. When e10s enabled, removing /chrome/userContent.css stops crashing.
2. When using /chrome/userContent.css, disabling e10s stops crashing.
(In reply to Eiichi from comment #69)
> After landing hg.mozilla.org/mozilla-central/, latest Central
> (20160817030202
> https://hg.mozilla.org/mozilla-central/rev/
> fe895421dfbe1f1f8f1fc6a39bb20774423a6d74) crashes with e10s enabled when
> using userContent.css.
> 
> 1. When e10s enabled, removing /chrome/userContent.css stops crashing.
> 2. When using /chrome/userContent.css, disabling e10s stops crashing.

Same changeset here and everything works fine, fresh built and using userContent.css.
(In reply to Eiichi from comment #69)
> After landing hg.mozilla.org/mozilla-central/, latest Central
> (20160817030202
> https://hg.mozilla.org/mozilla-central/rev/
> fe895421dfbe1f1f8f1fc6a39bb20774423a6d74) crashes with e10s enabled when
> using userContent.css.
> 
> 1. When e10s enabled, removing /chrome/userContent.css stops crashing.
> 2. When using /chrome/userContent.css, disabling e10s stops crashing.

Cannot reproduce, here the official build of Firefox 51.0a1 (2016-08-17) works fine and userContent.css indeed works (thanks wei :) . I'm using Ubuntu 16.04.1LTS x86_64.

- Any other idea to isolate your problem?
- Does using userContent.css crash with a fresh profile too?
- Do you get more crash info if you run Central from a terminal?
Depends on: 1295990
(In reply to Ronan Jouchet from comment #71)
> - Does using userContent.css crash with a fresh profile too?

Yes.

I tested:
1. Delete old profile(~/Library/Application Support/Firefox/).
2. Launch Central and make fresh profile(~/Library/Application Support/Firefox/Profiles/***/).
3. Quit Central.
4. Make empty file named userContent.css at ~/Library/Application Support/Firefox/Profiles/***/chrome/.
5. Launch Central.
6. I get "Bad news first: This tab has crashed.
Seems like Bug 1295990.
Depends on: 1296089
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #68)
> If this patch does not cause further regression then I think at least we can
> uplift this to 50.
> 
> :dbaron, do you think we can uplift to 49 or 48?

It's not ready to uplift anywhere until the regressions from it are fixed.  I think that probably means it shouldn't be uplifted to 49.
Flags: needinfo?(dbaron)
status-firefox51: fixed → affected
Target Milestone: mozilla51 → ---
Tried to change here
https://dxr.mozilla.org/mozilla-central/rev/052656fc513c05da969590ac5934abd67271a897/layout/style/Loader.cpp#1462
to

if (aLoadData->mSyncLoad && XRE_IsParentProcess()) {

but then the sheet is not loaded.
I guess I need a nsICSSLoaderObserver.
I've made a WIP that will open file:// remotely (i.e. request parent process to open it and send fd back) via nsFileChannel.
I hope this will solve sandbox issue.

Now I'm struggling with css parsing though. I guess I should not use LoadSheetSync in this case.
DevTool will try to open the file, which will fail.

Part 2 fixes some fatal exceptions (which will break the whole client UI),
but there are some other exceptions:

1. resolving original location will throw exception in server side
2. trying to modify the rule will get a exception, talking about it does not have a document.
Comment on attachment 8796446 [details]
Bug 1046166 - Add sandbox white list for userContent.css on MacOSX.

https://reviewboard.mozilla.org/r/82312/#review81356

::: devtools/client/inspector/rules/views/rule-editor.js:255
(Diff revision 1)
>      } else {
>        sourceLabel.setAttribute("value", title);
>        if (this.rule.ruleLine === -1 && this.rule.domRule.parentStyleSheet) {
>          this.source.setAttribute("unselectable", "true");
>        }
> +      if (sourceHref === "about:UserContentStyleSheet") {

This requires a comment just like the one we have above `if (sourceHref === "about:PreferenceStyleSheet") {` a few lines up.

::: devtools/server/actors/styles.js:995
(Diff revision 1)
>             (this._parentSheet &&
>              // If a rule does not have source, then it has been modified via
>              // CSSOM; and we should fall back to non-authored editing.
>              // https://bugzilla.mozilla.org/show_bug.cgi?id=1224121
>              this.sheetActor.allRulesHaveSource() &&
>              // Special case about:PreferenceStyleSheet, as it is generated on

This comment should not mention only about:PreferenceStyleSheet since the code below isn't only about this one stylesheet.
Instead maybe:
Special cases for stylesheets that are generated on the fly and for which URIs are not registered with about:handler, see https://bugzilla.mozilla.org/show_bug.cgi?id=935803#c37

::: devtools/server/actors/stylesheets.js:844
(Diff revision 1)
>     *
>     * @return boolean
>     *         Whether the stylesheet should be listed.
>     */
>    _shouldListSheet: function (doc, sheet) {
>      // Special case about:PreferenceStyleSheet, as it is generated on the

Same here.
Attachment #8796446 - Flags: review?(pbrosset) → review+
Attachment #8773650 - Flags: review+ → review?(dbaron)
Will external resources in userContent still work? (I don't think so because the new patch will send only userContent.css content to the child.)
Flags: needinfo?(wpan)
(In reply to Masatoshi Kimura [:emk] from comment #84)
> Will external resources in userContent still work? (I don't think so because
> the new patch will send only userContent.css content to the child.)

It can access resources from http(s)://, but not file://.
Resources from file:// simply won't load.
Flags: needinfo?(wpan)
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #85)
> (In reply to Masatoshi Kimura [:emk] from comment #84)
> > Will external resources in userContent still work? (I don't think so because
> > the new patch will send only userContent.css content to the child.)
> 
> It can access resources from http(s)://, but not file://.
> Resources from file:// simply won't load.

My userContent.css loads external resources from the same directory as where userContent.css is on (that is, file:// url.)
Do we drop support for file:// external resources, then?
Flags: needinfo?(wpan)
(In reply to Masatoshi Kimura [:emk] from comment #87)
> Do we drop support for file:// external resources, then?

Until we have implemented bug 922481, I'm afraid so.

You can convert those files to data-url as a workaround.
Flags: needinfo?(wpan)
:dbaron, should I choose another peer to reduce your review queue?
If so, which peer should I choose instead?

Thank you.
Flags: needinfo?(dbaron)
Comment on attachment 8773650 [details]
Bug 1046166 - Send userContent.css URL to content processes.

https://reviewboard.mozilla.org/r/66392/#review85680

Sorry for taking so long to get to this.

I'm marking it review- because of the code duplication in Loader.cpp.  Otherwise I'd be OK with it, although you should talk to somebody else (maybe bzbarsky) about whether the new fake about: URL is OK.

::: dom/ipc/ContentParent.cpp:2940
(Diff revision 4)
> +  rv = NS_NewLocalFileInputStream(getter_AddRefs(stream), file);
> +  if (NS_FAILED(rv)) {
> +    return;
> +  }
> +
> +  char chunk[8192] = "";

(actually, ignore this comment given my NS_ConsumeStream comment)

Leave this uninitialized rather than initializing to "".

::: dom/ipc/ContentParent.cpp:2940
(Diff revision 4)
> +  char chunk[8192] = "";
> +  uint32_t length = 0;
> +  nsAutoCString buffer;
> +  for (;;) {
> +    rv = stream->Read(chunk, sizeof(chunk), &length);
> +    if (NS_FAILED(rv)) {
> +      return;
> +    }
> +
> +    buffer.Append(chunk, length);
> +    if (length < sizeof(chunk)) {
> +      break;
> +    }
> +  }

Instead of this loop, just write:

nsCString buffer;
NS_ConsumeStream(stream, UINT32_MAX, buffer);

::: dom/ipc/ContentParent.cpp:2942
(Diff revision 4)
> +    return;
> +  }
> +
> +  char chunk[8192] = "";
> +  uint32_t length = 0;
> +  nsAutoCString buffer;

There's not much point in this being nsAutoCString rather than nsCString.  Odds are the file will either be empty/missing or be more than 64 bytes.  Just use nsCString.

::: dom/ipc/ContentParent.cpp:2950
(Diff revision 4)
> +    if (NS_FAILED(rv)) {
> +      return;
> +    }
> +
> +    buffer.Append(chunk, length);
> +    if (length < sizeof(chunk)) {

(ignore this given my NS_ConsumeStream comment)

If you interpret the documentation of nsIInputStream literally, this should just test length == 0.  (Yes, that will cause you to call Read one extra time, but I think that's ok.  There might be cases where read might not fill the buffer even when not at EOF.

(Then again, I feel like that shouldn't be the case for file streams... although I could imagine it being true for reading a compressed JAR.)

::: dom/ipc/ContentParent.cpp:3025
(Diff revision 4)
>        rv.SuppressException();
>        return false;
>      }
>    }
>  
> +  // XXX bug 1046166

Not clear why this says "XXX bug 1046166" if this patch is fixing bug 1046166.  This form of comment should refer to a bug that's still open.  I think you should just delete this line.

::: dom/ipc/PContent.ipdl:737
(Diff revision 4)
>          returns (bool isOffline, bool isConnected, bool isLangRTL,
>                   bool haveBidiKeyboards, nsString[] dictionaries,
>                   ClipboardCapabilities clipboardCaps,
>                   DomainPolicyClone domainPolicy,
> -                 StructuredCloneData initialData);
> +                 StructuredCloneData initialData,
> +                 nsCString userContentStyle);

Any API taking nsCString should document whether it's taking UTF-8, arbitrary bytes, or something else.  In this case I believe it's bytes, since it's just the raw contents of the file, I think.  (But you should make sure you agree.)

::: layout/style/Loader.h:334
(Diff revision 4)
>     */
>    nsresult LoadSheetSync(nsIURI* aURL, RefPtr<StyleSheet>* aSheet) {
>      return LoadSheetSync(aURL, eAuthorSheetFeatures, false, aSheet);
>    }
>  
> +  nsresult LoadSheetSync(nsIURI* aURL, const nsACString& aStyleString, RefPtr<StyleSheet>* aSheet);

Add a comment saying that this variant is used for loading user style sheets in the content process.

::: layout/style/Loader.cpp:2324
(Diff revision 4)
> +static nsresult
> +DetectCharset(const nsACString& aSegment, nsACString& aCharset)
> +{

I'm not happy about this much code duplication.

At the very least, this should be called DetectUserSheetCharset since it's not used for most charset detection.


However, I think you can avoid most of this duplication by:
 - adding a nsCString parameter to Loader::InternalLoadNonDocumentSheet
 - passing that parameter to a new parameter to SheetLoadData's constructor, which becomes a SheetLoadData member variable
 - using that parameter in the mSyncLoad case inside LoadSheet to call NS_NewCStringInputStream and NS_NewInputStreamChannel
 - using the rest (hopefully) of the existing code


Also, please prefer nsCSubstring or nsCString as types rather than nsACString.

::: layout/style/nsLayoutStylesheetCache.h:69
(Diff revision 4)
>  
>    static void InvalidatePreferenceSheets();
>  
>    static void Shutdown();
>  
> +  static void SetUserContentStyle(const nsACString& aStyle);

comment that this is called in the child process with the userContent sheet received from the parent.

::: layout/style/nsLayoutStylesheetCache.cpp:349
(Diff revision 4)
>                 &mXULSheet, eAgentSheetFeatures);
>  
> +  if (!gUserContentStyle.IsEmpty()) {
> +    MOZ_ASSERT(XRE_IsContentProcess(), "Only used in content processes.");
> +    BuildUserContentSheet();
> +    gUserContentStyle = EmptyCString();

Just write gUserContentStyle.Truncate()

::: layout/style/nsLayoutStylesheetCache.cpp:980
(Diff revision 4)
> +nsLayoutStylesheetCache::BuildUserContentSheet()
> +{
> +  MOZ_ASSERT(XRE_IsContentProcess(), "Only used in content processes.");
> +
> +  nsCOMPtr<nsIURI> uri;
> +  NS_NewURI(getter_AddRefs(uri), "about:UserContentStyleSheet", nullptr);

I'm not sure what the security implications of making up a new about: URI are.  You should find out before landing.

::: layout/style/nsLayoutStylesheetCache.cpp:983
(Diff revision 4)
> +  auto& loader = mBackendType == StyleBackendType::Gecko ?
> +    gCSSLoader_Gecko :
> +    gCSSLoader_Servo;
> +
> +  if (!loader) {
> +    loader = new mozilla::css::Loader(mBackendType);
> +    if (!loader) {
> +      ErrorLoadingBuiltinSheet(uri, "no Loader");
> +      return;
> +    }
> +  }

Please factor this into an EnsureLoader() function called by both LoadSheet and this, rather than copying it.
Attachment #8773650 - Flags: review?(dbaron) → review-
Flags: needinfo?(dbaron)
Wei-Cheng, do we have a path based on the review?
Flags: needinfo?(wpan)
(In reply to Milan Sreckovic [:milan] from comment #91)
> Wei-Cheng, do we have a path based on the review?

I'll need some time to revise the patch.
Since bug 922481 is still planning, I'd prefer to land this patch first, and open another bug that depends on it.

:bz, will we have security issue if we add a new fake about: url in this case?
Flags: needinfo?(wpan) → needinfo?(bzbarsky)
I don't think there should be any particular security issues, but it will lead to everyone having to know about the URL and special-case it (e.g. see the devtools changes you had to make) and will of course break all existing relative links in userContent sheets.

Is there a problem with simply giving it the relevant file:// URL as we have now?  The relative links may still not work due to bug 922481, but the devtools code may not need changes, since I assume devtools can read file:// URLs.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #93)
> Is there a problem with simply giving it the relevant file:// URL as we have
> now?  The relative links may still not work due to bug 922481, but the
> devtools code may not need changes, since I assume devtools can read file://
> URLs.

It may crash because sandboxing (e.g. profile directory for Mac OS or network drive for Windows).
Ah, the relevant code is actually running in the content process?

That's rather unfortunate...

In any case, this would be no different than our existing "about:PreferenceStyleSheet", which I guess has existing devtools hacks.  Ideally we would have something more systematic here, but we really don't want to be making up new URL schemes.  So "about:UserContentStyleSheet" should be fine.
Attachment #8773650 - Flags: review?(xidorn+moz)
I cannot review it. This really should be reviewed again by dbaron, although he's blocking review request... Let me try ni? him for this...
Flags: needinfo?(dbaron)
I made a change for the last review comment, I will push again when he is open for review request.

ongoing try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86f9c7a60ce3102395bfd0e78bd0f8a2eff51576
I don't know why review board plugin gave me "abort: server error when publishing review requests" error, even if I didn't add the reviewer field.
So I just upload the patch as an attachment.
Attachment #8773650 - Attachment is obsolete: true
I suspect this bug does not need to depend on bug 922481.  Unless you need write access to a file in the child, sandboxing for now is not preventing file:// reads from the child (read-only file access is still OK).

If I'm wrong feel free to re-mark the dependency.
No longer depends on: 922481
Flags: needinfo?(wpan)
(In reply to Jason Duell [:jduell] (needinfo me) from comment #101)
> I suspect this bug does not need to depend on bug 922481.  Unless you need
> write access to a file in the child, sandboxing for now is not preventing
> file:// reads from the child (read-only file access is still OK).
> 
> If I'm wrong feel free to re-mark the dependency.

Please read bug 1296089 and bug 1295990 that caused backout of the initial landing.
Depends on: 922481
Flags: needinfo?(wpan)
Changing dependency per bug 922481 comment #11.
Depends on: 1147911
No longer depends on: 922481
Jet, do we have an alternative to :dbaron reviewing the patch?
Flags: needinfo?(bugs)
Adding a sandbox white list now seems work for Mac OS (I don't know why it didn't work at that time though).
I'll test this approach on Windows as well, if that works too, then we can probably use the original patch.
Ongoing try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23fc70e26e13a17c1eeee0004e016ae6feaceb25

Tested on Mac (which have the profile in ~/Library) and Windows (which have the profile on a network location, e.g. \\SHARE\some\profile), and it can load resources in $PROFILE/chrome.
Temporary cancel the ni flag until the patch is ready.
Flags: needinfo?(dbaron)
Attachment #8806208 - Attachment is obsolete: true
Attachment #8806208 - Attachment is patch: false
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33031468c89afa4f13b5538467302b99ee0ad390

The first patch is almost the same to the backed patch.
Flags: needinfo?(dbaron)
Comment on attachment 8773650 [details]
Bug 1046166 - Send userContent.css URL to content processes.

https://reviewboard.mozilla.org/r/66392/#review93890

To my knowledge the chrome dir in the $PROFILE doesn't hold anthing security/privacy sensitive so this seems fine.
Attachment #8773650 - Flags: review+
Comment on attachment 8796446 [details]
Bug 1046166 - Add sandbox white list for userContent.css on MacOSX.

https://reviewboard.mozilla.org/r/82312/#review93900
Attachment #8796446 - Flags: review?(haftandilian) → review+
Attachment #8773650 - Flags: review+
Comment on attachment 8811654 [details]
Bug 1046166 - Add sandbox white list for userContent.css on Windows.

https://reviewboard.mozilla.org/r/93692/#review94168

I'm not sure this is the best place to put this, but I'm going to have to revisit this soon to add read access to other parts of the profile directory, so this is probably OK for now.
Attachment #8811654 - Flags: review?(bobowencode) → review+
Comment on attachment 8773650 [details]
Bug 1046166 - Send userContent.css URL to content processes.

https://reviewboard.mozilla.org/r/66392/#review94356

Seems like the only difference between https://hg.mozilla.org/mozilla-central/raw-rev/d1d614497fbf and https://reviewboard-hg.mozilla.org/gecko/raw-rev/c31568af155e6dd050010bf909ba913b2b7f82dc is the change of the |ucs| variable from StyleSheetHandle::RefPtr to StyleSheet* to match https://hg.mozilla.org/mozilla-central/rev/0938bc1e608f09461f4f18c13d6ae925c0e29b72 , which doesn't really require re-review.

However, I'm a little curious that MozReview thinks my previous review status was review-.  I guess that was comment 90, whereas I review+'d a patch in comment 60.  Not that MozReview will actually show me what the patch that I reviewed was in any diffable way.  So I'll just trust you that the patch landed in comment 66 was what I reviewed in comment 60, and mark this as review+.  I've made no attempt to verify this since it's hard to get old patch state out of MozReview.

Due to the combination of bug 1224733, bug 1251455, and MozReview's
inability to inspect old patch state in a usable way (some combination
of bugs like bug 1234279, bug 1249468, bug 1296135), I'm no longer
accepting large review requests in MozReview.  I'll give you a pass
this once since I didn't promptly provide feedback asking you to
post the patch again, but the basic problem is that MozReview is not
usable for re-review of patches.
Attachment #8773650 - Flags: review?(dbaron) → review-
(And I only filed *one* MozReview bug, bug 1318808, trying to re-review this patch.)
Flags: needinfo?(dbaron)
Comment on attachment 8773650 [details]
Bug 1046166 - Send userContent.css URL to content processes.

https://reviewboard.mozilla.org/r/66392/#review94358

Er, set the flag wrong (I think by not touching it at all, actually!?).
Attachment #8773650 - Flags: review- → review+
Pushed by wpan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73ed02918ff7
Send userContent.css URL to content processes. r=dbaron,haik
https://hg.mozilla.org/integration/autoland/rev/783d76ec78f0
Add sandbox white list for userContent.css on MacOSX. r=haik,pbro
https://hg.mozilla.org/integration/autoland/rev/a2ca13b26bba
Add sandbox white list for userContent.css on Windows. r=bobowen
The profile directory name includes a permanent unique identifier (e.g. "p69geh1t.default") -- by sending the URL to the content process, does this mean websites can see this?

Also, the URL would include the individual's username from their local system (e.g. /home/JohnSmith/.mozilla/firefox/p69geh1t.default/chrome/userContent.css) -- another potential privacy issue if this information is accessible to the website.

Can we clarify whether or not the userContent.css URL is visible to the website?
Flags: needinfo?(wpan)
https://hg.mozilla.org/mozilla-central/rev/73ed02918ff7
https://hg.mozilla.org/mozilla-central/rev/783d76ec78f0
https://hg.mozilla.org/mozilla-central/rev/a2ca13b26bba
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Is this backported to Firefox 52? Fx52 will be the next ESR version, so this may affect to company users.
> Can we clarify whether or not the userContent.css URL is visible to the website?

It's not.
Flags: needinfo?(wpan)
Actually, Wei-Chang, could you take a look at comment 121?
Flags: needinfo?(wpan)
Flags: needinfo?(bugs)
(In reply to gliwheple from comment #119)
> The profile directory name includes a permanent unique identifier (e.g.
> "p69geh1t.default") -- by sending the URL to the content process, does this
> mean websites can see this?
> 
> Also, the URL would include the individual's username from their local
> system (e.g.
> /home/JohnSmith/.mozilla/firefox/p69geh1t.default/chrome/userContent.css) --
> another potential privacy issue if this information is accessible to the
> website.
> 
> Can we clarify whether or not the userContent.css URL is visible to the
> website?

The URL is sending by an internal IPC message, so it will not be visible to websites.

(In reply to YUKI "Piro" Hiroshi from comment #121)
> Is this backported to Firefox 52? Fx52 will be the next ESR version, so this
> may affect to company users.

I think we can uplift this patch to Aurora (i.e. 52) at the very least, as long as it does not cause any further regression.

As for Beta, we may need some data (e.g. e10s statistics data) to decide.
Flags: needinfo?(wpan)
[Tracking Requested - why for this release]: We may want to uplift this.
status-firefox52: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Just to confirm, you should only need to uplift the first patch [1].

The sandbox on Windows Fx52 and below shouldn't prevent reading from anywhere.
The Mac one is only on Nightly at the moment and the one that is planned to be uplifted to 52, will again allow reads.

[1] Bug 1046166 - Send userContent.css URL to content processes.
Flags: needinfo?(wpan)
Fix confirmed using an official build of Firefox Nightly 53.0a1 (2016-11-22) (64-bit) under Ubuntu Linux 16.04.1. Thanks!
(In reply to Bob Owen (:bobowen) from comment #126)
> Just to confirm, you should only need to uplift the first patch [1].
> 
> The sandbox on Windows Fx52 and below shouldn't prevent reading from
> anywhere.
> The Mac one is only on Nightly at the moment and the one that is planned to
> be uplifted to 52, will again allow reads.
> 
> [1] Bug 1046166 - Send userContent.css URL to content processes.

Is the remote drive issue (bug 1295990) fixed?
(In reply to Masatoshi Kimura [:emk] from comment #128)

> Is the remote drive issue (bug 1295990) fixed?

That should only be affecting Nightly.
(In reply to Bob Owen (:bobowen) from comment #126)
> Just to confirm, you should only need to uplift the first patch [1].
> 
> The sandbox on Windows Fx52 and below shouldn't prevent reading from
> anywhere.

I found that it will crash if security.sandbox.content.level == 1. (for Nightly as well)
To make it work, I have to add
restricted_token.AddRestrictingSidCurrentUser()
to USER_NON_ADMIN at least.

:bobowen, do you have any suggestion? I was expecting level 1 will be looser then level 2.
Flags: needinfo?(wpan) → needinfo?(bobowencode)
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #130)
> (In reply to Bob Owen (:bobowen) from comment #126)
> > Just to confirm, you should only need to uplift the first patch [1].
> > 
> > The sandbox on Windows Fx52 and below shouldn't prevent reading from
> > anywhere.
> 
> I found that it will crash if security.sandbox.content.level == 1. (for
> Nightly as well)
> To make it work, I have to add
> restricted_token.AddRestrictingSidCurrentUser()
> to USER_NON_ADMIN at least.
> 
> :bobowen, do you have any suggestion? I was expecting level 1 will be looser
> then level 2.

Ah, right in theory it is, but the type of access token is slightly different, so that can cause problems in some circumstances.
Let me have a look at this.
After discussing with :wcpan, we both think it's not ready to uplift to 51. Track 51- and mark 51 won't fix.
status-firefox51: affected → wontfix
tracking-firefox51: ? → -
(In reply to Bob Owen (:bobowen) from comment #131)
> (In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #130)
> > (In reply to Bob Owen (:bobowen) from comment #126)
> > > Just to confirm, you should only need to uplift the first patch [1].
> > > 
> > > The sandbox on Windows Fx52 and below shouldn't prevent reading from
> > > anywhere.
> > 
> > I found that it will crash if security.sandbox.content.level == 1. (for
> > Nightly as well)
> > To make it work, I have to add
> > restricted_token.AddRestrictingSidCurrentUser()
> > to USER_NON_ADMIN at least.
> > 
> > :bobowen, do you have any suggestion? I was expecting level 1 will be looser
> > then level 2.
> 
> Ah, right in theory it is, but the type of access token is slightly
> different, so that can cause problems in some circumstances.
> Let me have a look at this.

What USER_INTERACTIVE is different from USER_NON_ADMIN [1] is

restricted_token.AddRestrictingSid(WinBuiltinUsersSid);
restricted_token.AddRestrictingSid(WinWorldSid);
restricted_token.AddRestrictingSid(WinRestrictedCodeSid);
restricted_token.AddRestrictingSidCurrentUser();
restricted_token.AddRestrictingSidLogonSession();

It does match what Wei-Cheng mentioned:

1) Higher sandbox level behaves less restrictive against some securable object.
2) Manually adding current user sid as "restricting sid" made it work.

If I understand |SidToRestrict| [2] (when calling CreateRestrictedToken) correctly,
adding new restricting sid to the restricted token might allow access to the
restricted token while the "original token" will be denied in case that the DACL
doesn't have any ACE against the added restricting sid:

"The Access Check performed for a restricted token against an object consists hence of two phases. The 1st phase consists of checking the list of "regular" SIDS contained in the token against the DACL and, if access has not been denied, the restricted SIDs against the access-allowed SIDs of the DACL. The access granted will consists of the most restrictive of the two." [3]

So, if 

1) the securable object doesn't have any ACE for the "current user sid" and 
2) the original access token doesn't have "current user sid"

the weird thing will happen.

[1] https://dxr.mozilla.org/mozilla-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc#64
[2] https://msdn.microsoft.com/zh-tw/library/windows/desktop/aa446583(v=vs.85).aspx
[3] http://leastprivilege.blogspot.tw/search/label/CreateRestrictedToken
Do we have to uplift the "Add sandbox white list for userContent.css on Windows" patch, after all? Or will we fix the root cause and uplift the fix?
(In reply to Masatoshi Kimura [:emk] from comment #134)
> Do we have to uplift the "Add sandbox white list for userContent.css on
> Windows" patch, after all? Or will we fix the root cause and uplift the fix?

We will need to fix the access token permission, and uplift the patch that added sandbox rule for Windows as well, according to my experiment.
(In reply to Bob Owen (:bobowen) from comment #131)
> (In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #130)

> > :bobowen, do you have any suggestion? I was expecting level 1 will be looser
> > then level 2.
> 
> Ah, right in theory it is, but the type of access token is slightly
> different, so that can cause problems in some circumstances.
> Let me have a look at this.

OK, here's what is happening.
The share that you are setting up and using for testing seems to be a bit strange (i.e. a share on the local computer).

Using both USER_NON_ADMIN and USER_INTERACTIVE cause the loading of the userContent.css to hit [1].
Because the in process call to NtQueryAttributes fails.

With USER_INTERACTIVE the status is 0xC0000022 (STATUS_ACCESS_DENIED) and so it doesn't return and instead asks the broker (parent process) for access, which is granted because of the rule you set up.

With USER_NON_ADMIN the status is 0xC0000201 (STATUS_NETWORK_OPEN_RESTRICTION), so the failure is returned and the load fails.

By adding in restricted_token.AddRestrictingSidCurrentUser() to USER_NON_ADMIN, you're making it a restricted token with less access rights than USER_INTERACTIVE and so you get the same behaviour, but it would break other things.

It's not clear to me why the Chromium sandbox only checks for STATUS_ACCESS_DENIED instead of using NT_SUCCESS(), but it's been like this since the initial commit of the code in their repository (as far as I can see).
I'll see if I can find out.

Are you able to test with a proper network share on a different machine, if we can reproduce the same problem then we might need to consider changing that check in the Chromium code and we would then need to uplift the rule as well.
(My testing of this worked with USER_NON_ADMIN.)

If there's a problem can you file another bug blocking this one.

Also, I think you should file an additional bug for the fact that failing to load the userContent.css crashes the content process (I think it would also crash the browser in non-e10s.)
It seems to me we should pass aParsingMode into ErrorLoadingBuiltinSheet and only crash if it is eAgentSheetFeatures.

[1] https://hg.mozilla.org/mozilla-central/file/a05726163a79/security/sandbox/chromium/sandbox/win/src/filesystem_interception.cc#l196
[2] https://hg.mozilla.org/mozilla-central/file/73ed02918ff7/layout/style/nsLayoutStylesheetCache.cpp#l745
Flags: needinfo?(bobowencode) → needinfo?(wpan)
No longer depends on: 1147911
(In reply to Bob Owen (:bobowen) from comment #136)
> (In reply to Bob Owen (:bobowen) from comment #131)
> > (In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #130)
> 
> > > :bobowen, do you have any suggestion? I was expecting level 1 will be looser
> > > then level 2.
> > 
> > Ah, right in theory it is, but the type of access token is slightly
> > different, so that can cause problems in some circumstances.
> > Let me have a look at this.
> 
> OK, here's what is happening.
> The share that you are setting up and using for testing seems to be a bit
> strange (i.e. a share on the local computer).
> 
> Using both USER_NON_ADMIN and USER_INTERACTIVE cause the loading of the
> userContent.css to hit [1].
> Because the in process call to NtQueryAttributes fails.
> 
> With USER_INTERACTIVE the status is 0xC0000022 (STATUS_ACCESS_DENIED) and so
> it doesn't return and instead asks the broker (parent process) for access,
> which is granted because of the rule you set up.
> 
> With USER_NON_ADMIN the status is 0xC0000201
> (STATUS_NETWORK_OPEN_RESTRICTION), so the failure is returned and the load
> fails.
> 
> By adding in restricted_token.AddRestrictingSidCurrentUser() to
> USER_NON_ADMIN, you're making it a restricted token with less access rights
> than USER_INTERACTIVE and so you get the same behaviour, but it would break
> other things.
> 
> It's not clear to me why the Chromium sandbox only checks for
> STATUS_ACCESS_DENIED instead of using NT_SUCCESS(), but it's been like this
> since the initial commit of the code in their repository (as far as I can
> see).
> I'll see if I can find out.
> 
> Are you able to test with a proper network share on a different machine, if
> we can reproduce the same problem then we might need to consider changing
> that check in the Chromium code and we would then need to uplift the rule as
> well.
> (My testing of this worked with USER_NON_ADMIN.)
> 
> If there's a problem can you file another bug blocking this one.

Tested sharing cross different machines, and it works.
Looks like the only problem is accessing from the same machine.

> Also, I think you should file an additional bug for the fact that failing to
> load the userContent.css crashes the content process (I think it would also
> crash the browser in non-e10s.)
> It seems to me we should pass aParsingMode into ErrorLoadingBuiltinSheet and
> only crash if it is eAgentSheetFeatures.

Good idea. I'll open a bug for this.
Flags: needinfo?(wpan)
Could anyone summarize what patches need uplifting to 52 to fix this issue?  And what's blocking them?  Thanks.
tracking-firefox52: ? → +
Comment #138.
Flags: needinfo?(wpan)
Flags: needinfo?(bobowencode)
(In reply to Julien Cristau [:jcristau] from comment #138)
> Could anyone summarize what patches need uplifting to 52 to fix this issue? 
> And what's blocking them?  Thanks.

My belief is still that we only need the first patch, but I also think we should at least have the follow-up so that failure to load the userContent.css doesn't crash.

From the comments it seems that crash was introduced for the internal css where failure to load would probably mean a non-functioning browser.
Flags: needinfo?(bobowencode)
Once bug 1320651 landed we can uplift bug 1320651 and the first patch in this bug to Fx 52.
Flags: needinfo?(wpan)
Comment on attachment 8773650 [details]
Bug 1046166 - Send userContent.css URL to content processes.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1046166
[User impact if declined]: They will not be able to use userContent.css, which was working in non-e10s.
[Is this code covered by automated tests?]: No, but has been cooked in nightly for awhile.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: With bug 1320651, tabs will not crash even if the sheet is blocked by the sandbox.
[String changes made/needed]:
Attachment #8773650 - Flags: approval-mozilla-aurora?
Depends on: 1322634
Comment on attachment 8773650 [details]
Bug 1046166 - Send userContent.css URL to content processes.

make userContent.css work with e10s, in aurora52
Attachment #8773650 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(wpan)
landed thanks!
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.