Use channel->asyncOpen2 in parser/htmlparser/nsExpatDriver.cpp

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM: Security
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Depends on: 2 bugs, 4 keywords)

unspecified
mozilla45
addon-compat, csectype-disclosure, privacy, sec-moderate
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [adv-main45-][post-critsmash-triage])

Attachments

(4 attachments, 6 obsolete attachments)

7.81 KB, patch
Details | Diff | Splinter Review
3.35 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
863 bytes, patch
Details | Diff | Splinter Review
8.58 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
Blocks: 1182535
(Assignee)

Comment 1

2 years ago
Created attachment 8636235 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch
Attachment #8636235 - Flags: review?(jonas)
Comment on attachment 8636235 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

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

::: dom/security/nsContentSecurityManager.cpp
@@ +138,5 @@
> +      {
> +        if (requestingContext) {
> +          nsCOMPtr<nsIDocument> doc = do_QueryInterface(requestingContext);
> +          MOZ_ASSERT(doc != nullptr,
> +                     "type_dtd requires requestingContext of type doc");

Something like MOZ_ASSERT(requestingContext->NodeType() == nsIDOMNode::DOCUMENT_TYPE) would be smaller/faster.
Attachment #8636235 - Flags: review?(jonas) → review+
(Assignee)

Comment 3

2 years ago
Comment on attachment 8636235 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

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

::: parser/htmlparser/nsExpatDriver.cpp
@@ +788,5 @@
>  
>    CopyUTF8toUTF16(absURL, aAbsURL);
>  
>    nsCOMPtr<nsIChannel> channel;
>    if (doc) {

We should do the same as in nsPluginHost, we could add something like:
NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
and eliminate that if-else branch, because a nullPrincipal is most likely not going to pass any security checks.
(Assignee)

Comment 4

2 years ago
Within nsExpatDriver.cpp there was no call to CheckLoadURIWithPrincipal(), now that we are adding the flag SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS we are performing a CheckLoadURIWithPrincipal() within the securityManager [1] which fails.

One of the tests that is failing [2] uses the following arguments for CheckLoadURIWithPrincipal().
  loadingPrincipal: about:feeds
  aURI: resource://gre/res/dtd/htmlmathml-f.ent

Note sure how we can fix that problem, because we have to set one of the 5 securityFlags defined in LoadInfo.
Jonas, any suggestions?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#72
[2] browser/base/content/test/general/test_bug364677.html
Flags: needinfo?(jonas)
If we don't want to do any security checks, then we should simply use the systemPrincipal as loadingPrincipal.

I think there is some other code somewhere that disables DTD loading almost everywhere. That might be enough, though we'd need to find where that code is. Peterv might know.

Another option is if we can configure things such that the resource://gre/res/dtd/htmlmathml-f.ent URL is loadable from web content. I think the chrome registry has the ability to tell web-accessible URLs from non-web-accessible ones. I don't know if we have something similar for resource URLs. I don't know who knows this part of the code. Jst maybe.
Flags: needinfo?(jonas)
(Assignee)

Comment 6

2 years ago
(In reply to Jonas Sicking (:sicking) from comment #5)
> If we don't want to do any security checks, then we should simply use the
> systemPrincipal as loadingPrincipal.

We could do that, which obviously works.

> I think there is some other code somewhere that disables DTD loading almost
> everywhere. That might be enough, though we'd need to find where that code
> is. Peterv might know.

Peter, do you happen to know where that code is?
Flags: needinfo?(peterv)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 7

2 years ago
Created attachment 8673405 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

This bug has been lying around for quite some time now. Let's keep that moving again. Rebased the patch. Carrying over r+ from Jonas and cancelling the needinfo for peterv.
Flags: needinfo?(peterv)
Attachment #8673405 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8636235 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
Comment on attachment 8673405 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

Johnny, probably you can help us out here: Can we use the Systemprincipal as the loadingPrincipal within nsExpatDriver? Please see also comment 5 from Jonas.
Attachment #8673405 - Flags: review?(jst)
cc:ing bholley and bz who likely know more about this than I do.
Looking at this function, we do the load in two cases:

1)  aURLStr led to a chrome:// URI.  
  or
2)  We managed to map aFPIStr to a known local DTD url.

Here we're falling into the second case.

But this whole thing looks totally broken to me.  Consider this XML file:

  <!DOCTYPE window [
  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
  %brandDTD;
  ]>
  <window>&brandShorterName;</window>

When I load that from some random website, that website gets to sync-load that DTD and then can see what &brandShorterName; resolves to in it.  This seems totally broken to me.

I tracked this back a bit and this "allow all loads for chrome:// dtd files" fates back to revision 3.1 of nsExpatDriver.cpp, back in 2002 (patch by harishd, r=heikki, very old times).

It looks like we added the content policy call in bug 418119 but no CheckLoadURI call...

So I think what we should do is only treat the loading principal as system in the "known local DTD url" case: those DTDs are basically specified in various web standards.  In the other cases, we should be using the principal of the document, or the null principal if there is no document (assuming that can happen at all).  And we should probably replace that misleading isChrome check with a check for the UI_RESOURCE or LOCAL_RESOURCE flag, since it's really just checking that it's OK to syncload from that URI.
Group: dom-core-security
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> (In reply to Jonas Sicking (:sicking) from comment #5)
> > I think there is some other code somewhere that disables DTD loading almost
> > everywhere. That might be enough, though we'd need to find where that code
> > is. Peterv might know.
> 
> Peter, do you happen to know where that code is?

Sorry, missed this needinfo. I don't really understand what code you're looking for. nsExpatDriver::OpenInputStreamFromExternalDTD is the code that loads external DTDs, what else do you need?
Flags: needinfo?(mozilla)
(Assignee)

Updated

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

Comment 12

2 years ago
Created attachment 8673988 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

Boris, is this patch doing what you had in mind or are we missing something?
Attachment #8673405 - Attachment is obsolete: true
Attachment #8673405 - Flags: review?(jst)
Attachment #8673988 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 13

2 years ago
(In reply to Peter Van der Beken [:peterv] from comment #11)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> > (In reply to Jonas Sicking (:sicking) from comment #5)
> > > I think there is some other code somewhere that disables DTD loading almost
> > > everywhere. That might be enough, though we'd need to find where that code
> > > is. Peterv might know.
> > 
> > Peter, do you happen to know where that code is?
> 
> Sorry, missed this needinfo. I don't really understand what code you're
> looking for. nsExpatDriver::OpenInputStreamFromExternalDTD is the code that
> loads external DTDs, what else do you need?

Sorry Peter, I just realized I accidentally cleared the needinfo flag earlier this afternoon. That was not my intention. I think Boris is having a clear vision of what we should do within this patch, so solely relying on the systemPrincipal as proposed in comment 5 is not an option anymore.
Comment on attachment 8673988 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

All the things that are UI_RESOURCE are also LOCAL_RESOURCE, so there is no point in checking both flags.  Pick one or the other, depending on what semantics we want to enforce here.  UI_RESOURCE is likely better, since it's more restrictive.

However returning NS_ERROR_FAILURE if not UI_RESOURCE is wrong, because the stuff that LookupCatalogData needs to get passed to it is not UI_RESOURCE; it's http:// URIs.  So what you want to do is if not UI_RESOURCE do the LookupCatalogData thing.

Apart from that this looks reasonable, I think.
Attachment #8673988 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 15

2 years ago
Created attachment 8675936 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

Hey Boris, here is an updated version. Please note that I added 'nsILoadInfo::SEC_ALLOW_CHROME' as a securityflag within the else branch. Reason is, when running test [1] we are loading:
> channelURI: chrome://global/locale/global.dtd
> loadingPrincipal: about:feeds
> triggeringPrincipal: about:feeds
> contentPolicyType: 13

which would fail the CheckLoadURIWithPrincipal() check within the contentSecurityManager [2]. I suppose that's the right thing to do, but I would like you to confirm. Anything else we missed here?

[1] browser/base/content/test/general/test_bug364677.html
[2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#57
Attachment #8673988 - Attachment is obsolete: true
Attachment #8675936 - Flags: review?(bzbarsky)
Comment on attachment 8675936 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

So the new behavior is that if !isUIResource we try to GetLocalDTDURI and if that fails we still do the load, no?

That's wrong.  The whole point of the existing checks here is that if !isUIResource and !localURI then we should not do the load.  Please add tests as needed, since clearly we have poor test coverage here.
Attachment #8675936 - Flags: review?(bzbarsky) → review-
Comment on attachment 8675936 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

Oh, and the ALLOW_CHROME bit is probably OK as long as that restricts to exposed chrome packages.  I can't tell from the documentation in nsILoadInfo.idl what the flag actually does, so can't comment intelligently on its use.  :(
(Assignee)

Comment 18

2 years ago
Created attachment 8685164 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

Boris, I added back that check:
> if (!localURI) {
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
so that the load is denied if !isUIResource and !localURI.
Attachment #8675936 - Attachment is obsolete: true
Attachment #8685164 - Flags: review?(bzbarsky)
(Assignee)

Comment 19

2 years ago
Created attachment 8685166 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests.patch

I am not sure if I am testing the right thing here, so please let me know if I am mistaken. My guess would have been that ALLOW_CHROME allows to pass CheckLoadURIWithPrincipal() but then the load gets denied because "chrome://branding/locale/brand.dtd" is not accessible to content. In other words, I don't see 'contentaccessible=yes' here [1].

The test currently fails because the current setup allows loading "chrome://branding/locale/brand.dtd". I am not sure how to proceed at this point. Am I missing something?

[1] http://mxr.mozilla.org/mozilla-central/source/browser/branding/nightly/locales/jar.mn#10
Attachment #8685166 - Flags: feedback?(bzbarsky)
Comment on attachment 8685164 [details] [diff] [review]
bug_1182546_asyncopen2_nsexpatdriver.patch

r=me.  Here's hoping I didn't miss anything....
Attachment #8685164 - Flags: review?(bzbarsky) → review+
Comment on attachment 8685166 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests.patch

> The test currently fails

Currently as in on tip right now, or currently as in with the AsyncOpen2 patch?
Flags: needinfo?(mozilla)
(Assignee)

Comment 22

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #21)
> > The test currently fails
> 
> Currently as in on tip right now, or currently as in with the AsyncOpen2
> patch?

Sorry for not being precise. With 'currently' I meant: With the asyncopen2()-patch from this bug applied the test fails. In other words, we are allowing to access "chrome://branding/locale/brand.dtd" even though we shouldn't so that browserShortName translates into 'Nightly'.

Running the test without the asyncOpen2() patch from this bug applied, we get the same result, the test fails and we are allowing to access "chrome://branding/locale/brand.dtd" even though we shouldn't.
Flags: needinfo?(mozilla)
Comment on attachment 8685166 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests.patch

This test looks reasonable to me.  I don't understand why it fails.  Per IRC discussion, Christoph will try to figure that out.
Attachment #8685166 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 24

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #23)
> This test looks reasonable to me.  I don't understand why it fails.  Per IRC
> discussion, Christoph will try to figure that out.

As discussed on IRC the test is failing because %content/branding/ is indeed contentAccessible [1]. Only removing contentaccessible=yes from [1] is not doing the trick though. I don't know where that package lives and [2] confuses me even more :-(.

Anyway, you were right, we are creating the new channel in the 'else' branch where we pass 'SEC_ALLOW_CHROME' as a securityFlag. Here are the important values of the loadinfo:
> channelURI: chrome://branding/locale/brand.dtd
> loadingPrincipal: http://mochi.test:8888/tests/browser/base/content/test/general/bug_1182546.xml
> triggeringPrincipal: http://mochi.test:8888/tests/browser/base/content/test/general/bug_1182546.xml
> contentPolicyType: 13

Since the package is content accessible, the function CheckLoadURIWithPrincipal() returns true here [3] and allows the load.

You suggested we could file a bug to make %content/branding not accessible to content. Would you like that to happen within this bug? File a separate bug and make it block this one? Or would you like to rewrite the test to rely on a different URI?


[1] http://mxr.mozilla.org/mozilla-central/source/browser/branding/nightly/content/jar.mn#6
[2] http://mxr.mozilla.org/mozilla-central/search?string=%25content%2Fbranding%2F&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
[3] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#814
Flags: needinfo?(bzbarsky)
For purposes of this bug, pick a different URI.  And file a separate bug, depending on this one, to move the DTD out of the content-exposed stuff.  Unless the Firefox folks are OK with pages reading all the stuff in that DTD, of course.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 26

2 years ago
Boris, by now I had quite so many attempts to make that test work, but I can't seem to find any URI that is not accessible from content, see some of my attempts underneath:

%---test1---
<!DOCTYPE html [
  <!ENTITY % aboutDTD SYSTEM "chrome://global/locale/about.dtd">
  %aboutDTD;
]>
<window>&about.credits.beforeLink;</window>


%---test2----
<!DOCTYPE window [
  <!ENTITY % canvasDebuggerDTD SYSTEM "chrome://devtools/locale/canvasdebugger.dtd">
	%canvasDebuggerDTD;
]>
<window>&canvasDebuggerUI.recordSnapshot.tooltip;</window>


%---test3----
<!DOCTYPE window [
<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
  %brandDTD;
]>
<window>&brandShorterName;</window>


The good news is that the patch works correctly; if I remove the contentaccessible=yes from unofficial/content/jar.mn [1] then the *.dtd gets blocked correctly with the patch applied but loads without the patch applied.

Any suggestions on what URI we could use?


[1] http://mxr.mozilla.org/mozilla-central/source/browser/branding/unofficial/content/jar.mn#6
Flags: needinfo?(bzbarsky)
I have to admit that I don't understand why the contentaccessible annotation for content/branding affects a dtd in branding/locale...  Given that, I'm really nor sure how to tell which dtds are not contentaccessible.  Gijs, do you have any idea?
Flags: needinfo?(bzbarsky) → needinfo?(gijskruitbosch+bugs)

Comment 28

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #27)
> I have to admit that I don't understand why the contentaccessible annotation
> for content/branding affects a dtd in branding/locale...  Given that, I'm
> really nor sure how to tell which dtds are not contentaccessible.  Gijs, do
> you have any idea?

This is expected behaviour per https://developer.mozilla.org/en/docs/Chrome_Registration#contentaccessible , specifically:

> The contentaccessible flag applies only to content packages: it is not
> recognized for locale or skin registration. However, the matching locale
> and skin packages will also be exposed to content.


For the test, really the most stable thing to do would be to register your own bootstrapped manifest and remove it again when you're done. The tests for the chrome registry ( https://dxr.mozilla.org/mozilla-central/source/chrome/test ) can probably help with figuring out how to do this. Specifically, https://dxr.mozilla.org/mozilla-central/source/chrome/test/unit/head_crtestutils.js#9 when used from an xpcshell test should work. Because autoRegister persists for the remainder of the executable's lifetime, you probably want to use this method instead if you're using mochitests: https://dxr.mozilla.org/mozilla-central/source/xpcom/components/nsIComponentManager.idl#81 .

I realize this is more work, but I broke the tree the other day because we have tests in caps that rely on arbitrary details of how we package firefox (specifically, on a file called chrome://global/skin/passwordmgr/key.png or something actually existing), and Android is now fighting those same tests again as they move to "finally" remove a bunch of unused XUL/CSS from the fennec packaging. I'd really really really prefer not to exacerbate this problem.

If this is somehow completely impossible, if you look at the toolkit manifest the build system generates:

https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/bin/chrome/toolkit.manifest#5

It seems like a locale file from any of the (corresponding locale) packages without contentaccessible=yes should get the job done.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 29

2 years ago
Created attachment 8688089 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests.patch

Gijs, thanks for your help. Registering a manifest seems complicated. However, I've tried and I am willing to invest a little more time if you can guide me. Please have a look at the other patch (which I am going to upload in a minute) before you review this patch. The testcase in this patch is the 'baseline' test which we could use in case we don't get the registration of the manifest working.
Attachment #8685166 - Attachment is obsolete: true
Attachment #8688089 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 30

2 years ago
Created attachment 8688095 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests_2.patch

Gijs, so this testcase is at a very half baked state at this point, but at least the basic components should be there. I am trying to register a new manifest using
> SpecialPowers.Components.manager.addBootstrappedManifestLocation(MANIFEST_FILE);

which throws:

2 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/test_bug1182546.html | uncaught exception - Error: Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass at http://mochi.test:8888/tests/browser/base/content/test/general/test_bug1182546.html:32
    simpletestOnerror@SimpleTest/SimpleTest.js:1519:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1506:1


But I suppose that's how the whole dance should somehow work, right? It's the first time I have to deal with packaging for Firefox, so any help is highly appreciated. Thanks!
Attachment #8688095 - Flags: feedback?(gijskruitbosch+bugs)

Comment 31

2 years ago
Comment on attachment 8688095 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests_2.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> Created attachment 8688095 [details] [diff] [review]
> bug_1182546_nsexpatdriver_tests_2.patch
> 
> Gijs, so this testcase is at a very half baked state at this point, but at
> least the basic components should be there. I am trying to register a new
> manifest using
> > SpecialPowers.Components.manager.addBootstrappedManifestLocation(MANIFEST_FILE);
> 
> which throws:
> 
> 2 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/test_bug1182546.html | uncaught exception
> - Error: Permission denied for <http://mochi.test:8888> to create wrapper
> for object of class UnnamedClass at
> http://mochi.test:8888/tests/browser/base/content/test/general/
> test_bug1182546.html:32
>     simpletestOnerror@SimpleTest/SimpleTest.js:1519:11
>     OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1506:1
> 
> 
> But I suppose that's how the whole dance should somehow work, right? It's
> the first time I have to deal with packaging for Firefox, so any help is
> highly appreciated. Thanks!

Seems like you should use a browser mochitest instead of a plain one? Is there some reason that won't work?
Flags: needinfo?(mozilla)
Attachment #8688095 - Flags: feedback?(gijskruitbosch+bugs)
(Assignee)

Comment 32

2 years ago
(In reply to :Gijs Kruitbosch from comment #31)
> Seems like you should use a browser mochitest instead of a plain one? Is
> there some reason that won't work?

Mainly because I wanted to simulate that a 'user' can't access a chrome package if it's not accessible to content. I suppose a browser chrome test would work.
Flags: needinfo?(mozilla)

Comment 33

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #32)
> (In reply to :Gijs Kruitbosch from comment #31)
> > Seems like you should use a browser mochitest instead of a plain one? Is
> > there some reason that won't work?
> 
> Mainly because I wanted to simulate that a 'user' can't access a chrome
> package if it's not accessible to content. I suppose a browser chrome test
> would work.

Right. You would add the manifest in the test, then load a page in-content that tests the accessibility of the locale file.

Comment 34

2 years ago
Comment on attachment 8688089 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests.patch

If it's going to speed things up here, considering this is a security issue, land this and deal with the "proper" test in a followup?
Attachment #8688089 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 35

2 years ago
Comment on attachment 8688095 [details] [diff] [review]
bug_1182546_nsexpatdriver_tests_2.patch

(In reply to :Gijs Kruitbosch from comment #34)
> If it's going to speed things up here, considering this is a security issue,
> land this and deal with the "proper" test in a followup?

That sounds like a good idea to me - thanks!
Attachment #8688095 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: csectype-disclosure, privacy, sec-moderate
(Assignee)

Comment 36

2 years ago
Created attachment 8689247 [details] [diff] [review]
bug_1182546_nsexpatdriver_make_test_contentaccessible.patch

Boris, test [1] is failing because inlinesettings is not marked to be accessible from content which makes CheckLoadURIWithPrincipal bail out with an error not allowing the load [2].

[1] toolkit/mozapps/extensions/test/browser/browser_inlinesettings_custom.js
[2] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#856
Attachment #8689247 - Flags: review?(bzbarsky)
Comment on attachment 8689247 [details] [diff] [review]
bug_1182546_nsexpatdriver_make_test_contentaccessible.patch

This is a test-only addon?  r=me
Attachment #8689247 - Flags: review?(bzbarsky) → review+
Hi, this got backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17518341&repo=mozilla-inbound
Flags: needinfo?(mozilla)
(Assignee)

Comment 39

2 years ago
Created attachment 8689879 [details] [diff] [review]
bug_1182546_nsexpatdriver_make_branding_contentaccessible.patch

(In reply to Carsten Book [:Tomcat] from comment #38)
> Hi, this got backed out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=17518341&repo=mozilla-
> inbound

Gijs, at the moment it seems there is an inconsistency between browser/, b2g/ and mobile/ where branding is accessible from content for browser/ [1] but not for b2g/ and mobile/ which caused the patches of this bug to be backed out.

I agree with Boris that we should indeed figure out whether the .dtd needs to be accessible from content or not (See Comment 24, 25 and 26). For this bug however, I think it's fine that branding is accessible from content not only for browser/ but also for b2g/ and mobile/.

TRY looks good [2]. Let me know if you feel differently and we can discuss our options again.

[1] http://mxr.mozilla.org/mozilla-central/search?string=content%2Bbranding
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a9baaf72709
Flags: needinfo?(mozilla)
Attachment #8689879 - Flags: review?(gijskruitbosch+bugs)

Comment 40

2 years ago
Comment on attachment 8689879 [details] [diff] [review]
bug_1182546_nsexpatdriver_make_branding_contentaccessible.patch

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

rs=me
Attachment #8689879 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

2 years ago
Depends on: 1226823
(Assignee)

Updated

2 years ago
Depends on: 1226869
https://hg.mozilla.org/mozilla-central/rev/9c2bad72beab
https://hg.mozilla.org/mozilla-central/rev/5c551d0f4bfe
https://hg.mozilla.org/mozilla-central/rev/529d0520902c
https://hg.mozilla.org/mozilla-central/rev/792401624285
status-firefox45: --- → fixed
Target Milestone: --- → mozilla45

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Summary: Use channel->ascynOpen2 in parser/htmlparser/nsExpatDriver.cpp → Use channel->asyncOpen2 in parser/htmlparser/nsExpatDriver.cpp
Keywords: addon-compat
Depends on: 1227554
Depends on: 1227981
(Assignee)

Updated

2 years ago
Depends on: 1228116
Group: dom-core-security → core-security-release
Depends on: 1228117
No longer depends on: 1228117
Whiteboard: [adv-main45-]
Depends on: 1228117
Whiteboard: [adv-main45-] → [adv-main45-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.