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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Jorg K (CEST = GMT+2)]

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.

Comment 1

[Jorg K (CEST = GMT+2)]

[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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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...

Comment 5

[Jorg K (CEST = GMT+2)]

(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]

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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?

Comment 9

[:Gijs (he/him)]

(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?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 12

[Jorg K (CEST = GMT+2)]

(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.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 14

[:Gijs (he/him)]

(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.

Comment 15

[:Gijs (he/him)]

(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.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(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?

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 18

[:Gijs (he/him)]

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.

Comment 19

[:Gijs (he/him)]

(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.

Comment 20

[Jorg K (CEST = GMT+2)]

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?

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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?

Comment 23

[Jorg K (CEST = GMT+2)]

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!

Comment 24

[Jorg K (CEST = GMT+2)]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

This test is wrong.  addOverrideStyleSheet should always work.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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!

Comment 30

[Boris Zbarsky [:bzbarsky]]

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.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> 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

[Boris Zbarsky [:bzbarsky]]

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!

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

(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!

Comment 37

[Richard Marti (:Paenglab)]

Thank you Christoph, TB's compose works again with this patch.

Comment 38

[Jorg K (CEST = GMT+2)]

Please land this asap. Thanks again!

Comment 41

[aleth [:aleth]]

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

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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!

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66a1404b9902

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9535c05d5022

Comment 47

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/66a1404b9902
https://hg.mozilla.org/mozilla-central/rev/9535c05d5022

Comment 49

[aleth [:aleth]]

(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?

Comment 50

[Boris Zbarsky [:bzbarsky]]

I was expecting that Christoph would do that...

Comment 51

[:Gijs (he/him)]

(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.

Comment 52

[Christoph Kerschbaumer [:ckerschb]]

(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!

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I don't see any reason to track this bug now that it's fixed.