Restore addOverrideStyleSheet() to working order, regression of bug 1195173

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Jorg K (GMT+2), Assigned: ckerschb)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla47
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47- fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
We noticed in Thunderbird that addOverrideStyleSheet() has stopped working for us on the style sheets we need to load:

Loading chrome://editor/content/EditorContent.css
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2424
Loading chrome://messenger/skin/messageQuotes.css
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4933
Loading chrome://messenger/content/composerOverlay.css

Also see Boris' comment in bug 1195173 comment #62.
(Reporter)

Updated

2 years ago
Blocks: 1195173
Keywords: regression
(Reporter)

Comment 1

2 years ago
[Tracking Requested - why for this release]:
addOverrideStyleSheet() was damaged today by bug 1195173, so it seems obvious that this needs to be fixed. In fact, it might make sense to back out bug 1195173 until it's fixed properly.
tracking-firefox47: --- → ?

Updated

2 years ago
Assignee: nobody → mozilla
Component: DOM → DOM: Security
Flags: needinfo?(mozilla)

Comment 2

2 years ago
Jorg, if you have a local tree that can reproduce the problem, does making Loader::CheckContentPolicy return NS_OK immediately if !aSourcePrincipal fix the issue?
Flags: needinfo?(mozilla)
(Assignee)

Comment 3

2 years ago
I am only guessing here, but since we use asyncOpen2() for stylesheets now, we have to pass a securityFlag other than Components.interfaces.nsILoadInfo.SEC_NORMAL to newChannelFromURI2 within MsgComposeCommands.js. Since that channel uses the systemPrincipal, we should in fact use the flag: Components.interfaces.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL.

Even better would be to rewrite [1]

let channel = NetUtil.newChannel({
  uri: url,
  loadUsingSystemPrincipal: true
});

I am out of the office today unfortunately - if that doesn't fix it, please let me know.

[1] https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3996
Flags: needinfo?(mozilla)

Comment 4

2 years ago
Christoph, the code you link to has nothing to do with the code loading the stylesheets.  That code lives entirely in layout/style/Loader.cpp and is setting whatever flags it's setting...

Updated

2 years ago
Flags: needinfo?(mozilla)
(Reporter)

Comment 5

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #2)
> Jorg, if you have a local tree that can reproduce the problem, does making
> Loader::CheckContentPolicy return NS_OK immediately if !aSourcePrincipal fix
> the issue?
I have various local trees on various machines ;-)

On one of them I changed layout/style/Loader.cpp as suggested:
nsresult
Loader::CheckContentPolicy(nsIPrincipal* aSourcePrincipal,
                          nsIURI* aTargetURI,
                          nsISupports* aContext,
                          bool aIsPreload)
{
  if (!aSourcePrincipal) return NS_OK;
  nsContentPolicyType contentPolicyType =
    aIsPreload ? nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD
               : nsIContentPolicy::TYPE_INTERNAL_STYLESHEET;

This does NOT fix the problem. I'm still seeing:
JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 2424: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIEditorStyleSheets.addOverrideStyleSheet]
Flags: needinfo?(mozilla)
(Assignee)

Comment 6

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #4)
> Christoph, the code you link to has nothing to do with the code loading the
> stylesheets.  That code lives entirely in layout/style/Loader.cpp and is
> setting whatever flags it's setting...

Indeed, that was in my rush yesterday - the newChannel part should be rewritten anyway, because that will definitely lead to further problems soon.
(Assignee)

Comment 7

2 years ago
Created attachment 8715939 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

Boris, it seems to me that the basic functionality for addOverrideStyleSheet() is working properly. Since we are adding ALLOW_CHROME as a security-flag, chrome:// urls should be accessible (if allowed within our packaging machinery).

Is it possible that "chrome://messenger/skin/messageQuotes.css" is somehow *not* accessible to content[1] for Thunderbird?

At the moment I don't think that contentPolicies are blocking that load, but unfortunately my testcase is only half baked at the moment, because I use "chrome://messenger/skin/messageQuotes.css" for the second part of the test which is not available on mozilla-central at all (only on comm-central). However, if blocked by contentPolicies, I should see it at that point.

I would need a chrome://*.css URL that is not allowed to be loaded from content to test properly and figure out where we are blocking that load. Could you guide me to one?

I don't really see any differences between [1] and [2] in terms of packaging that would block messageQuotes.css from loading on Thunderbird but would allow chrome:// urls to be loaded on mozilla-central.

[1] http://mxr.mozilla.org/comm-central/source/mail/themes/linux/jar.mn#30
[2] http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/jar.mn#34
Flags: needinfo?(mozilla) → needinfo?(bzbarsky)

Comment 8

2 years ago
Christoph, you probably want to talk to Jorg about what's going on here and steps to reproduce....

> Could you guide me to one?

I don't know offhand.  Gijs, do you?
Flags: needinfo?(bzbarsky) → needinfo?(gijskruitbosch+bugs)

Comment 9

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #8)
> Christoph, you probably want to talk to Jorg about what's going on here and
> steps to reproduce....
> 
> > Could you guide me to one?
> 
> I don't know offhand.  Gijs, do you?

data:text/html,<link rel="stylesheet" href="chrome://mozapps/skin/aboutNetworking.css"><link>

on current Firefox 45 beta gives me:

Security Error: Content at moz-nullprincipal:{d612aa94-f4ff-9d41-b6f0-56121d839f6b} may not load or link to chrome://mozapps/skin/aboutNetworking.css.

in the browser console (but not the web console)

Does that help?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mozilla)
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1245897
(Assignee)

Comment 11

2 years ago
Created attachment 8716012 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

(In reply to :Gijs Kruitbosch from comment #9) 
> data:text/html,<link rel="stylesheet"
> href="chrome://mozapps/skin/aboutNetworking.css"><link>

> Does that help?

For the test I am writing yes. Fixing the actual problem seems to be more tricky. I am seeing the following error now:
> Security Error: Content at http://mochi.test:8888/tests/editor/libeditor/tests/test_bug1245681.html may not load or link to chrome://mozapps/skin/aboutNetworking.css.

But that's not the error reported in this bug - not sure what goes wrong.
Attachment #8715939 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
(Reporter)

Comment 12

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #8)
> Christoph, you probably want to talk to Jorg about what's going on here and
> steps to reproduce....
The simplest way might be to build TB:
https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build
Basically you clone http://hg.mozilla.org/comm-central.
Then you add M-C as a subdirectory "mozilla". In the end you build with "mozilla/mach build". On a quick machine that takes under an hour (assuming that you don't have to clone M-C). To produce the problem, simply write a new e-mail message. The JS code controlling the message composition window does the call. 

Since the message composition window is really just a XUL document with an embedded contenteditable, perhaps you could mock up something similar, open it in FF and then do the call on page load or so. After all, there a XUL test in the system. Well, just fantasising.

If you want me to try a patch for you or do some debugging, let me know.
(Assignee)

Comment 13

2 years ago
Gijs, can you think of anything why 'chrome://messenger/skin/messageQuotes.css' [1] might not be accessible to content? I don't know enough about our packaging system to answer that question, but it seems it's packaged here: [2].

[1] https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2424
[2] http://mxr.mozilla.org/comm-central/source/mail/themes/linux/jar.mn#30
Flags: needinfo?(gijskruitbosch+bugs)

Comment 14

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Gijs, can you think of anything why
> 'chrome://messenger/skin/messageQuotes.css' [1] might not be accessible to
> content? I don't know enough about our packaging system to answer that
> question, but it seems it's packaged here: [2].
> 
> [1]
> https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/
> MsgComposeCommands.js#2424
> [2] http://mxr.mozilla.org/comm-central/source/mail/themes/linux/jar.mn#30

Yes, that is simple to answer: nothing chrome:// is content-accessible by default. The only way to make something content-accessible is to mark the content package's registration with contentaccessible=yes in the manifest (or jar.mn) file. We do this for a lot of Firefox UI things because of the in-content pages. There's a bug on file to separate that stuff out... anyway.

http://mxr.mozilla.org/comm-central/source/mail/base/jar.mn#3

registers the content package here, and doesn't have a contentaccessible flag, so it's not accessible to content.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 15

2 years ago
(In reply to :Gijs Kruitbosch from comment #14)
> There's a bug on file to separate that stuff out... anyway.

bug 443400, for completeness' sake.
(Assignee)

Comment 16

2 years ago
(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> > Gijs, can you think of anything why
> > 'chrome://messenger/skin/messageQuotes.css' [1] might not be accessible to
> > content? I don't know enough about our packaging system to answer that
> > question, but it seems it's packaged here: [2].
> > 
> > [1]
> > https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/
> > MsgComposeCommands.js#2424
> > [2] http://mxr.mozilla.org/comm-central/source/mail/themes/linux/jar.mn#30
> 
> Yes, that is simple to answer: nothing chrome:// is content-accessible by
> default. The only way to make something content-accessible is to mark the
> content package's registration with contentaccessible=yes in the manifest
> (or jar.mn) file. We do this for a lot of Firefox UI things because of the
> in-content pages. There's a bug on file to separate that stuff out... anyway.
> 
> http://mxr.mozilla.org/comm-central/source/mail/base/jar.mn#3
> 
> registers the content package here, and doesn't have a contentaccessible
> flag, so it's not accessible to content.

I suppose that is your answer Jorg - can you give that a try and add contentaccessible=yes? That should fix it!

Can you give that a try and let us know?
Flags: needinfo?(mozilla)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8716012 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

Since I already wrote the test for it, we could also land it, no?

Thanks for your help figuring this out Gijs!
Attachment #8716012 - Flags: review?(gijskruitbosch+bugs)

Comment 18

2 years ago
Comment on attachment 8716012 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

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

All of this is in editor, so I'm not sure I'm a good reviewer... but here's what I can tell you anyway. :-)

::: editor/libeditor/tests/mochitest.ini
@@ +166,5 @@
>  [test_bug1186799.html]
>  [test_bug1181130-1.html]
>  [test_bug1181130-2.html]
>  [test_backspace_vs.html]
> +[test_bug1245681.html]

Nit: personally, I would like us to start using test names that indicate what the test is doing.

::: editor/libeditor/tests/test_bug1245681.html
@@ +25,5 @@
> +  var editframe = window.frames[0];
> +  var editdoc = editframe.document;
> +  var editor = null;
> +
> +  editdoc.designMode='on';

nit: spacing

@@ +27,5 @@
> +  var editor = null;
> +
> +  editdoc.designMode='on';
> +
> +  editor = SpecialPowers.wrap(editframe)

nit: can just declare editor here instead of assigning null initially.

@@ +36,5 @@
> +                        .getEditorForWindow(editframe);
> +
> +  styleSheets = editor.QueryInterface(Ci.nsIEditorStyleSheets);
> +
> +  editdoc.designMode='off';

nit: spacing. But also, why are we turning it off again here?

@@ +42,5 @@
> +  // try to access chrome:// url that is allowed to load
> +  try
> +  {
> +    styleSheets.addOverrideStyleSheet("chrome://browser/content/pageinfo/pageInfo.css");
> +    ok(true, "should be allowed to access designmode.css");

This says designmode.css but you're using pageInfo.css, so now I'm confused.

@@ +51,5 @@
> +
> +  // try to access chrome:// url that is *not* allowed to load
> +  try
> +  {
> +    styleSheets.addOverrideStyleSheet("chrome://mozapps/skin/aboutNetworking.css");

This should ideally use a test file for the test. This stuff has bit me before when removing random icon files in our Linux theme, so I'd really prefer not to repeat the experience. Worse, I expect this might throw (as expected!) if the file is (re)moved, and then the test stops testing that this is working.

You can register and unregister a bootstrapped manifest using the components manager (addBootstrappedManifestLocation with nsIFile).

It might also make sense to verify the exception is a SecurityError one.
Attachment #8716012 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 19

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> I suppose that is your answer Jorg - can you give that a try and add
> contentaccessible=yes? That should fix it!
> 
> Can you give that a try and let us know?

FWIW, while that would test whether this works, it would also make all of chrome://messenger/*/* content-accessible.

It would be... wiser... to add a resource: URL registration for the stylesheet in question and load it using that, instead of exposing all of these files to content.
(Reporter)

Comment 20

2 years ago
OK, adding contentaccessible=yes at the end of this line
http://mxr.mozilla.org/comm-central/source/mail/base/jar.mn#3
so it becomes
% content messenger %content/messenger/ contentaccessible=yes
makes the problem go away.

Sadly I know nothing about this (being a C++ programmer).
I also noticed that changing
http://mxr.mozilla.org/comm-central/source/mail/themes/linux/jar.mn
also adding
% content messenger %content/messenger/ contentaccessible=yes
(or what should I have added?) made no difference.

Can you please explain why the things changed in bug 1195173 changed the behaviour. IN TB, the jar.mn file hasn't changed.

> It would be... wiser... to add a resource: URL registration for the stylesheet in
> question and load it using that, instead of exposing all of these files to content.
While I was debugging yesterday I saw some "chrome://" and something else go past, maybe it was
resource://gre/res/contenteditable.css, sorry I can't remember, I remember the "gre" bit ;-)
That's what you're referring to, right?
Flags: needinfo?(mozilla)
(Assignee)

Comment 21

2 years ago
(In reply to Jorg K (GMT+1) from comment #20)
> Can you please explain why the things changed in bug 1195173 changed the
> behaviour. IN TB, the jar.mn file hasn't changed.

I can answer that, but how to make things accessible to content correctly, we have to wait for Gijs response.

In general, we are working on a project we call 'security by default'. Probably you have seen that we convert channel to use asyncOpen2() instead of asyncOpen(). AsyncOpen() performs security checks in an 'opt-out' manner rather than an 'opt-in' manner that we had for historical reasons. Until now checkLoadUriWithPrincipal in your particular case was not enforced, but in fact it should have been. Now that we converted the styleLoader to use asyncOpen2() [see Bug 1195173] all loads enforce the same security checks and chrome:// URLs can only be loaded if marked 'contentaccessible=yes'. I hope that answers your question.
(Assignee)

Comment 22

2 years ago
Created attachment 8716056 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

(In reply to :Gijs Kruitbosch from comment #18)
> Comment on attachment 8716012 [details] [diff] [review]
> bug_1245681_fix_addoverwritestylesheet.patch

Gijs, thanks for your feedback. I incorporated your suggestions and nits.

> You can register and unregister a bootstrapped manifest using the components
> manager (addBootstrappedManifestLocation with nsIFile).

Even though I agree it would be nice to register a manifest and rewrite the test to accomplish that, it seems like a lot of work for not that much gain at the moment. I would rather keep the test simple and just rely on build in *.css files which are contentaccessible or not.



Boris, would you mind accepting such a test?
Attachment #8716012 - Attachment is obsolete: true
Attachment #8716056 - Flags: review?(bzbarsky)
(Reporter)

Comment 23

2 years ago
OK, thanks.

I suggest to do whatever you like with this bug. Close it as invalid, or use it to land your test. Maybe change the summary to your liking.

Thanks for diagnosing our problem. I opened a new TB bug 1246002 and we will fix the issue. Thanks again!
(Reporter)

Comment 24

2 years ago
Oh actually. The error code returned is somewhat suboptimal, is it not? Could that be improved?

Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIEditorStyleSheets.addOverrideStyleSheet]

Comment 25

2 years ago
No.  This is not OK.  addOverrideStyleSheet needs to actually work reliably.  That's why it used to skip the security checks.  That's why it's supposed to be using the system principal as the triggering principal.  If the problem is that it's using the wrong loading principal and we now check against that too, then we should fix the loading principal it uses.

Exposing random CSS to the world is NOT THE RIGHT SOLUTION.  Especially in a mail client, where "the world" means "all the emails you get".  We do NOT want to be leaking more information to those.  We just don't.

Comment 26

2 years ago
Comment on attachment 8716056 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

This test is wrong.  addOverrideStyleSheet should always work.
Attachment #8716056 - Flags: review?(bzbarsky) → review-

Comment 27

2 years ago
And note that this is not the first time when we introduce new stricter security checks during this project and as a result claim that we need to make stuff world-accessible when it really shouldn't be.  Again, the right answer is that the security checks need to be smarter, not that we should open everything to the world because the security checks are dumb on some specific codepaths.

Comment 28

2 years ago
Er, meant to cc sicking on comment 27.  My firm opinion is that all the contentaccessible=yes that got added during the AsyncOpen2 conversions should be backed out in favor of saner solutions.
(Assignee)

Comment 29

2 years ago
Created attachment 8716090 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

Boris, this patch would use the systemPrincipal as the loadingPrincipal (and also the triggeringPrincipal) for addOverrideStyleSheet(). Please note that when using this patch we would not bypass CheckContentPolicy(). But potentially we should contentPolicies after all anyway.

There are several other solutions on how we could fix this problem. We could set the systemPrincipal earlier, e.g. when creating SheetLoadData.

Before moving forward I wanted to ask how you would like to have it handled. Potentially we could even remove the boolean aUseSystemPrincipal-flag completely. Please let me know. Once we know a path forward here, I will update the test accordingly.

Thanks for your help!
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Updated

2 years ago
Blocks: 1246002

Comment 30

2 years ago
You can't remove mUseSystemPrincipal/aUseSystemPrincipal altogether, because it's used to set the actual principal of the sheet in SheetLoadData::OnStreamComplete.

I'm not entirely convinced we should key this solely off aUseSystemPrincipal.  Conceptually, it seems to me that pretty much any LoadSheetSync() call should be treated as a system load.  We have a bunch of loads like that for things like UA sheets and whatnot that are basically succeeding more or less by accident right now, looks like.

I guess my gut feeling is that if we get to LoadSheet (the version that takes a SheetLoadData) and aLoadData->mLoaderPrincipal is null, then we should use system for both loading _and_ triggering principal, not just for triggering principal as we do right now.

So basically, I think we should change the "aLoadData->mRequestingNode" condition to "aLoadData->mRequestingNode && aLoadData->mLoaderPrincipal".

Or never set mRequestingNode in Loader::InternalLoadNonDocumentSheet if !aOriginPrincipal, I guess.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 31

2 years ago
Created attachment 8716412 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

(In reply to Boris Zbarsky [:bz] from comment #30)
> You can't remove mUseSystemPrincipal/aUseSystemPrincipal altogether, because
> it's used to set the actual principal of the sheet in
> SheetLoadData::OnStreamComplete.

Well, we could in fact make sure that we always pass a principal and then rely on mLoaderPrincipal within onStreamComplete. If mLoaderPrincipal == systemPrincipal, then...
But anyway, even if you would consider that approach, it would be too much of a change for that bug in my opinion.

> I'm not entirely convinced we should key this solely off
> aUseSystemPrincipal.  Conceptually, it seems to me that pretty much any
> LoadSheetSync() call should be treated as a system load.  We have a bunch of
> loads like that for things like UA sheets and whatnot that are basically
> succeeding more or less by accident right now, looks like.
> 
> I guess my gut feeling is that if we get to LoadSheet (the version that
> takes a SheetLoadData) and aLoadData->mLoaderPrincipal is null, then we
> should use system for both loading _and_ triggering principal, not just for
> triggering principal as we do right now.

I suppose that statement not only applies to sync loads but also to async-loads, right? Within the patch I am using that approach for sync and async loads; passes all of the tests locally within layout/style/test. (haven't had it on TRY yet though).

> So basically, I think we should change the "aLoadData->mRequestingNode"
> condition to "aLoadData->mRequestingNode && aLoadData->mLoaderPrincipal".

Yes, that makes sense.

Finally, to completely restore the order of what we had before, I also brought back the change within contentPolicies which were bypassed |if (!aSourcePrincipal)|. I hope you agree.
Attachment #8716056 - Attachment is obsolete: true
Attachment #8716090 - Attachment is obsolete: true
Attachment #8716412 - Flags: review?(bzbarsky)
(Assignee)

Comment 32

2 years ago
Created attachment 8716413 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet_tests.patch

I rewrote that test to make sure addOverrideStyleSheet succeeds *not* depending on the chrome URL being accessible to content or not.
Attachment #8716413 - Flags: review?(bzbarsky)

Comment 33

2 years ago
> and then rely on mLoaderPrincipal within onStreamComplete. If mLoaderPrincipal == systemPrincipal, then...

Just because the _loader_ principal is system doesn't necessarily mean the _sheet_ principal should be.

> I suppose that statement not only applies to sync loads but also to async-loads, right?

In practice I believe our async loads should always have a source principal...

Comment 34

2 years ago
Comment on attachment 8716412 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet.patch

r=me.  Thank you for pushing back on what the desired behavior is here!
Attachment #8716412 - Flags: review?(bzbarsky) → review+

Comment 35

2 years ago
Comment on attachment 8716413 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet_tests.patch

r=me
Attachment #8716413 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 36

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #33)
> > and then rely on mLoaderPrincipal within onStreamComplete. If mLoaderPrincipal == systemPrincipal, then...
> 
> Just because the _loader_ principal is system doesn't necessarily mean the
> _sheet_ principal should be.

Ah, I see. Makes sense actually.


Thanks for your help and the speedy reviews!
Thank you Christoph, TB's compose works again with this patch.
(Reporter)

Comment 38

2 years ago
Please land this asap. Thanks again!
Keywords: checkin-needed

Updated

2 years ago
Duplicate of this bug: 1246002

Updated

2 years ago
Duplicate of this bug: 1246230

Comment 41

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b71df90db19e917b0195c89e76593c8312d2ec68
Bug 1245681 - Fix addOverrideStyleSheet. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/82803bc92f3772165cedaa29c1d110cc2c6bb9f7
Bug 1245681 - Test for addOverrideStyleSheet. r=bz

Updated

2 years ago
Keywords: checkin-needed
I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3f25f37acf for test failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=21170399&repo=mozilla-inbound
Flags: needinfo?(mozilla)
(Assignee)

Comment 43

2 years ago
Created attachment 8716509 [details] [diff] [review]
bug_1245681_fix_addoverwritestylesheet_tests.patch

The chrome:// URLs used for the test are not available on Android. We should just disable the test on Android in that case.

Wes, I am not entirely certain, but I think
> +[test_css_chrome_load_access.html]
> +skip-if = toolkit == 'android' # chrome urls not available due to packaging
is all I have to do to disable the test for Android, right?

If so, please feel free to reland those patches once inbound is open again. Sorry for the bustage!
Attachment #8716413 - Attachment is obsolete: true
Flags: needinfo?(mozilla) → needinfo?(wkocher)
Attachment #8716509 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/66a1404b9902
Flags: needinfo?(wkocher)

Updated

2 years ago
Keywords: checkin-needed

Comment 45

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9535c05d5022
Duplicate of this bug: 1246380
https://hg.mozilla.org/mozilla-central/rev/66a1404b9902
https://hg.mozilla.org/mozilla-central/rev/9535c05d5022
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Duplicate of this bug: 1246462

Comment 49

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #28)
> Er, meant to cc sicking on comment 27.  My firm opinion is that all the
> contentaccessible=yes that got added during the AsyncOpen2 conversions
> should be backed out in favor of saner solutions.

bz: Did you intend to file a bug for this?
Flags: needinfo?(bzbarsky)

Comment 50

2 years ago
I was expecting that Christoph would do that...
Flags: needinfo?(bzbarsky)

Updated

2 years ago
Depends on: 1246578

Comment 51

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #50)
> I was expecting that Christoph would do that...

I went ahead and poked about for these, then filed bug 1246578.
(Assignee)

Comment 52

2 years ago
(In reply to :Gijs Kruitbosch from comment #51)
> (In reply to Boris Zbarsky [:bz] from comment #50)
> > I was expecting that Christoph would do that...
> 
> I went ahead and poked about for these, then filed bug 1246578.

Thanks Gijs!
I don't see any reason to track this bug now that it's fixed.
tracking-firefox47: ? → -
You need to log in before you can comment on or make changes to this bug.