Closed Bug 1173199 Opened 5 years ago Closed 4 years ago

Add a pref to allow disabling MathML (Tor 13548)

Categories

(Core :: MathML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: arthur, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(1 file, 4 obsolete files)

To reduce attack surface area, in "high security" mode, Tor Browser disables MathML. This was achieved by adding a pref that disables MathML. We would like to propose upstreaming this pref to Firefox.
Here is the patch updated to mozilla-central. The pref is called 'mathml.disabled'.
Attachment #8617669 - Flags: review?(karlt)
Whiteboard: [tor]
Well if this make you feel better, but it leaves all the code in place so really does not lessen the attack surface at all.  Just sayin'!
(In reply to Bill Gianopoulos [:WG9s] from comment #2)
> Well if this make you feel better, but it leaves all the code in place so
> really does not lessen the attack surface at all.  Just sayin'!

When enabled, this pref blocks the MathML code from being exercised. So I think it does offer some protection from exploits against the MathML implementation.
> To reduce attack surface area, in "high security" mode, Tor Browser disables MathML.

Can you send me the link with the list of features that Tor Browser disables at "high security" mode?
Flags: needinfo?(arthuredelstein)
(In reply to Arthur Edelstein from comment #3)
> (In reply to Bill Gianopoulos [:WG9s] from comment #2)
> > Well if this make you feel better, but it leaves all the code in place so
> > really does not lessen the attack surface at all.  Just sayin'!
> 
> When enabled, this pref blocks the MathML code from being exercised. So I
> think it does offer some protection from exploits against the MathML
> implementation.

That is assuming you can't flip the pref or get into the code via some other means.  I get it but seems like would not stop a serious hacker from finding a way to exploit this code if it has vulnerabilities.
To reduce the attack surface you really need to provide a browser with the extra code for these disabled features not in the executable.  This does not reduce the attack surface it only makes it a bit more difficult to attack the existing either way attack surface.  Let's get our security terms right here.
TO make it clear I ma not saying this is a bad idea, just that saying it reduces the attack surface is misusing security terms.  the same attack surface still exists after this change just if it it p referenced off it is less accessible.  So this just makes part of the attack surface less accessible, it does not really reduce the attack surface.
(In reply to Raniere Silva from comment #4)
> > To reduce attack surface area, in "high security" mode, Tor Browser disables MathML.
> 
> Can you send me the link with the list of features that Tor Browser disables
> at "high security" mode?

See https://www.torproject.org/projects/torbrowser/design/#other-security

(I should have said MathML is disabled at "Medium-Low" security level and higher.)
Flags: needinfo?(arthuredelstein)
Arthur,

svg.in-content.enabled is a Tor Browser option only? If yes, could you point to the bug that propose upstreaming it to Firefox? I think that we should have upstream all the options needed by Tor Browser or have none just for the sake of consistency.
Flags: needinfo?(arthuredelstein)
Comment on attachment 8617669 [details] [diff] [review]
0001-Create-preference-to-disable-MathML.patch

I don't mind having preferences to disable certain features, but I don't know the appropriate way to do it, so I suggest getting a DOM peer to review.

Also have a look through other changes in http://hg.mozilla.org/mozilla-central/rev/b8664f450508
nsSVGFeatures::HaveExtension() should not say it supports MathML, I assume.
Attachment #8617669 - Flags: review?(karlt) → review?(bugs)
Comment on attachment 8617669 [details] [diff] [review]
0001-Create-preference-to-disable-MathML.patch

Please use AddBoolVarCache for observing changes to a bool pref, but add the 
VarCache only once, so protect it using a static variable.
There are lots of examples in the tree about the pattern.


* goes with the type, not variable name.
Attachment #8617669 - Flags: review?(bugs) → review-
(In reply to Arthur Edelstein from comment #8)
> See https://www.torproject.org/projects/torbrowser/design/#other-security
> 
> (I should have said MathML is disabled at "Medium-Low" security level and
> higher.)

I'm curious to know what was the reasoning to put MathML in that category and whether the Mozilla MathML/Security teams should do something to provide more security guarantee on MathML to Tor people.

The "Medium-Low" level seems to only disable features related to executable code (javascript & java) while MathML is essentially a complex rendering of text so it should be treated at the same level as layout, graphics & fonts (e.g. the latest public critical issue I'm aware of is https://www.mozilla.org/en-US/security/advisories/mfsa2014-59/, which is actually really from the "DirectWrite font handling").

The iSEC study does not even mention vulnerability of MathML while it says that "the SVG components have been the host of several exploitable bugs in the past several years" and recommends to "disable at the Low or Medium security level"... but your link says it is only disabled in High mode.

Also, the iSEC study says it rely on the exploit analysis, but a quick search on https://www.mozilla.org/en-US/security returns far less results (two) for MathML than for SVG. And actually a search for "graphite" also returns two crashes too: https://www.mozilla.org/en-US/security/advisories/mfsa2012-64/

Finally, the iSEC study seems to take into account the number of websites using a given feature, but MathML does not seem less popular than graphite or svg opentype fonts.
(In reply to Frédéric Wang (:fredw) from comment #12)
> (In reply to Arthur Edelstein from comment #8)
> > See https://www.torproject.org/projects/torbrowser/design/#other-security
> > 
> > (I should have said MathML is disabled at "Medium-Low" security level and
> > higher.)
> 
> I'm curious to know what was the reasoning to put MathML in that category
> and whether the Mozilla MathML/Security teams should do something to provide
> more security guarantee on MathML to Tor people.
> 
> The "Medium-Low" level seems to only disable features related to executable
> code (javascript & java) while MathML is essentially a complex rendering of
> text so it should be treated at the same level as layout, graphics & fonts
> (e.g. the latest public critical issue I'm aware of is
> https://www.mozilla.org/en-US/security/advisories/mfsa2014-59/, which is
> actually really from the "DirectWrite font handling").
> 
> The iSEC study does not even mention vulnerability of MathML while it says
> that "the SVG components have been the host of several exploitable bugs in
> the past several years" and recommends to "disable at the Low or Medium
> security level"... but your link says it is only disabled in High mode.
> 
> Also, the iSEC study says it rely on the exploit analysis, but a quick
> search on https://www.mozilla.org/en-US/security returns far less results
> (two) for MathML than for SVG. And actually a search for "graphite" also
> returns two crashes too:
> https://www.mozilla.org/en-US/security/advisories/mfsa2012-64/
> 
> Finally, the iSEC study seems to take into account the number of websites
> using a given feature, but MathML does not seem less popular than graphite
> or svg opentype fonts.

I think you are raising interesting points. I've posted them here: https://lists.torproject.org/pipermail/tbb-dev/2015-June/000284.html
Flags: needinfo?(arthuredelstein)
Shouldn't this be discussed on the https://groups.google.com/forum/#!forum/mozilla.governance mailing list?
(or maybe mozilla.dev.platform)
Why would this be a governance issue? Mozilla leadership has already decided to help Tor move toward being able to build off a Release Firefox rather than an ESR--it's safer for our users. I don't know if we'll get to the point where they can just ship a re-packaged bundle with some pref flips and add-ons, but the more of their patches we incorporate into mozilla-central the easier it will be for them to apply their remaining patches each release.

If on any given patch from Tor we reach an impasse between the relevant module owner and what the Tor folks want then sure, that issue can be raised to a broader audience on dev.platform or ultimately to the Module Owner King.

I'll note that for a long time Thunderbird disabled MathML and SVG (among other things), because mail doesn't (didn't?) need it and it reduced their attack surface. It's a valid approach to get rid of features you don't think your product needs.
(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8617669 [details] [diff] [review]
> 0001-Create-preference-to-disable-MathML.patch
> 
> Please use AddBoolVarCache for observing changes to a bool pref, but add the 
> VarCache only once, so protect it using a static variable.
> There are lots of examples in the tree about the pattern.

Thanks for the review. Here's a new version that uses AddBoolVarCache.
 
> * goes with the type, not variable name.

I've fixed this as well.

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=169351319888
Attachment #8617669 - Attachment is obsolete: true
Attachment #8624346 - Flags: review?(bugs)
Comment on attachment 8624346 [details] [diff] [review]
0001-Create-preference-to-disable-MathML.patch

>   if (ns == kNameSpaceID_MathML) {
>-    return NS_NewMathMLElement(aResult, ni.forget());
>+    // If the mathml.disabled pref. is true, convert all MathML nodes into
>+    // generic XML nodes by swapping the namespace.
>+    if (!sMathMLDisabled) {
>+      return NS_NewMathMLElement(aResult, ni.forget());
>+    }
>+    nsNodeInfoManager* niMgr = ni->NodeInfoManager();
>+    nsRefPtr<mozilla::dom::NodeInfo> genericXMLNI
>+      = niMgr->GetNodeInfo(ni->NameAtom(), ni->GetPrefixAtom(),
Nit, I'd put = to the end of the previous line.

>+          kNameSpaceID_XML, ni->NodeType(), ni->GetExtraName());
>+    return NS_NewXMLElement(aResult, genericXMLNI.forget());
>   }
But why can you just add && !sMathMLDisabled to 'if (ns == kNameSpaceID_MathML) {'
We would then fallback to the end of the method and call
return NS_NewXMLElement(aResult, ni.forget());
And for the sake of consistency, perhaps add a pref check also to HasElementCreator
Comment on attachment 8624346 [details] [diff] [review]
0001-Create-preference-to-disable-MathML.patch

and, how can you pass kNameSpaceID_XML as the namespace. That is not what should happen. The namespace should be still kNameSpaceID_MathML.
Hmm, this is tricky. We do rely on namespace to identify the actual class of an element.
Attachment #8624346 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8624346 [details] [diff] [review]
> 0001-Create-preference-to-disable-MathML.patch
> 
> and, how can you pass kNameSpaceID_XML as the namespace. That is not what
> should happen. The namespace should be still kNameSpaceID_MathML.
> Hmm, this is tricky. We do rely on namespace to identify the actual class of
> an element.

The spirit of this patch is to disable MathML by treating MathML elements as generic XML elements.  This was done by substituting a generic XML namespace for the MathML namespace.  Will this cause other problems?  What are the implications of "We do rely on namespace to identify the actual class of an element" ?
Flags: needinfo?(bugs)
Perhaps we should call the pref then something else, like
mathml.convert_to_null_namespace or some such, and then actually use null namespace (that is at least less wrong than xml namespace) so that created elements would be plain normal XML elements.

Since we do rely on namespace to map to certain type of element, using the same namespace for other
element types (with type I mean some C++ class) can't be done easily.
But do we want to disable MathML DOM elements, or just the layout?
If the latter, we could perhaps prevent creating mathml layout objects.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #18)

> But why can you just add && !sMathMLDisabled to 'if (ns ==
> kNameSpaceID_MathML) {'
> We would then fallback to the end of the method and call
> return NS_NewXMLElement(aResult, ni.forget());

I tried this fallback approach, but it fails to stop many MathML elements from being rendered as MathML.
(In reply to Olli Pettay [:smaug] from comment #22)
> Perhaps we should call the pref then something else, like
> mathml.convert_to_null_namespace or some such, and then actually use null
> namespace (that is at least less wrong than xml namespace) so that created
> elements would be plain normal XML elements.

I like the name 'mathml.disabled', because that's what we're aiming to do. If necessary we can change the implementation to better fit this description.

Here's a new version that sets the namespace to 'kNameSpaceID_Unknown'. This prevents MathML from being rendered. Calling it 'Unknown' seems appropriate, as we want the browser to pretend it is not aware of how to render MathML. Does this seem like an OK approach?

> Since we do rely on namespace to map to certain type of element, using the
> same namespace for other
> element types (with type I mean some C++ class) can't be done easily.
> But do we want to disable MathML DOM elements, or just the layout?
> If the latter, we could perhaps prevent creating mathml layout objects.

Our hope is for the pref to deactivate as much of the MathML C++ code as possible to reduce risk.
Attachment #8624346 - Attachment is obsolete: true
Attachment #8624937 - Flags: review?(bugs)
(Fixing a nit.)
Attachment #8624937 - Attachment is obsolete: true
Attachment #8624937 - Flags: review?(bugs)
Attachment #8624938 - Flags: review?(bugs)
(In reply to Arthur Edelstein from comment #24)
> Here's a new version that sets the namespace to 'kNameSpaceID_Unknown'. This
> prevents MathML from being rendered. Calling it 'Unknown' seems appropriate,
> as we want the browser to pretend it is not aware of how to render MathML.
> Does this seem like an OK approach?

Well, changing the namespace is rather weird. It says nothing about not able to render MathML, but just
that there is some very unusual namespace transformation happening. And that is why I don't like the pref 
name "mathml.disabled". Disabling mathml should leave the XML elements using the mathml namespace etc, but just disable all the specialness mathml has, so effectively disable layout.
And given that nsMathMLElement doesn't add too much functionality, I think disabling layout, and perhaps some behavior of 
nsMathMLElement would be better approach, and certainly closer to what a pref named "mathml.disabled" should mean.
Comment on attachment 8624938 [details] [diff] [review]
0001-Create-preference-to-disable-MathML.patch

So, I'm not really happy with this approach
(especially since I think the better approach isn't too hard to implement).

I could possibly live this kind of patch if the pref name was something else.
Attachment #8624938 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #26)
> (In reply to Arthur Edelstein from comment #24)
> > Here's a new version that sets the namespace to 'kNameSpaceID_Unknown'. This
> > prevents MathML from being rendered. Calling it 'Unknown' seems appropriate,
> > as we want the browser to pretend it is not aware of how to render MathML.
> > Does this seem like an OK approach?
> 
> Well, changing the namespace is rather weird. It says nothing about not able
> to render MathML, but just
> that there is some very unusual namespace transformation happening. And that
> is why I don't like the pref 
> name "mathml.disabled". Disabling mathml should leave the XML elements using
> the mathml namespace etc, but just disable all the specialness mathml has,
> so effectively disable layout.
> And given that nsMathMLElement doesn't add too much functionality, I think
> disabling layout, and perhaps some behavior of 
> nsMathMLElement would be better approach, and certainly closer to what a
> pref named "mathml.disabled" should mean.

OK, I can look into that option. Do you have any suggestions on how to disable MathML layout and nsMathMLElement efficiently? One thing I found attractive about the namespace approach is how simple it was.
(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8624938 [details] [diff] [review]
> 0001-Create-preference-to-disable-MathML.patch
> 
> So, I'm not really happy with this approach
> (especially since I think the better approach isn't too hard to implement).
> 
> I could possibly live this kind of patch if the pref name was something else.

Probably it's better not to sneak this in, though. :) I'm happy if there's a better approach.
I have been testing this today so thought I would put my name on this. Feel free to steal if I don't get anything for it soon.
Assignee: nobody → jkingston
My 2p.

Thanks smaug for the review.  I've asked Jonathan and Arthur to take a look at your solution to see if it is a better approach that both Mozilla and the Tor Project can accept.  Our primary goal is to un-fork the Tor Browser.  If we change their patches in a way that is unacceptable to the Tor Project, we're not achieving that goal.

I propose that all of the Tor patch related prefs be under "privacy.*".  I'm currently working on bug 1260931 and the pref I'm adding is "privacy.firstparty.isolate".  So for this patch, maybe we do "privacy.mathml.disable" or "privacy.mathml.debuff" or something.  Smaug what would be acceptable to you?

I agree with Arthur that changing the namespace is simple and therefore elegant.  I would like to land these patches as close to as-is as possible.  Un-forking is the name of the game. :)

--dave
Flags: needinfo?(bugs)
Flags: needinfo?(arthuredelstein)
I discussed with jkt yesterday, and I hope the approach I proposed would be both easy to implement and would make mathml still behave correctly as normal XML: namespace would stay the right one for JS , but internally we'd map that namespace to different ID so we wouldn't ever detect there to be
MathMLElements around since those wouldn't be created.
There was still mathml.css loading prevention, something in SVG and a case or two in browser UI where mathml namespace is used which would need a pref check.
But overall, assuming the approach works, it should be close to trivial, and wouldn't break namespaces.

Changing the namespace is just a breaking change and might break web sites relying on MathML (providing MathML support using scripts when native MathML implementation isn't detected.)
I wouldn't take the current patch in this bug, or the one which Tor uses, given that other approach is probably close to as simple and should work better. But of course need to test that approach.

I don't care too much about the pref names. privacy.mathml.disable is fine, so is mathml.disable
Flags: needinfo?(bugs)
If the mathml.disabled preference is true, treat <math> and other MathML
elements as generic XML elements.

This patch disables the rendering code of MathML however preserves the
namespace so to reduce the breakage.

Original patch by: Kathy Brade <brade@pearlcrescent.com>

Review commit: https://reviewboard.mozilla.org/r/61042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61042/
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/1-2/
Blocks: meta_tor
Hey :smaug,

So the two changesets are kind of the choices I have in implementation here:
1. https://hg.mozilla.org/try/rev/aadb83ab28699e28033d533c7ffb42df05463578
2. https://hg.mozilla.org/try/rev/c332d43b179c93be1696e744fc8b2979aa1b170a

1. moves around the issue that html is parsed differently (https://hg.mozilla.org/try/rev/aadb83ab28699e28033d533c7ffb42df05463578#l2.89) and assigns the namespace directly where as 2. modifies the parser(https://hg.mozilla.org/try/rev/c332d43b179c93be1696e744fc8b2979aa1b170a#l13.30) to change when it finds the element (granted this would need to be done in the java file as we discussed).

The differences are not required for xhtml which works completely with the namespace hacking.

I'm not entirely sure 1. gains the advantage you mentioned that the elements will still have the correct namespace.
Flags: needinfo?(bugs)
+NS_IMETHODIMP
+nsNameSpaceManager::Observe(nsISupports* aObject, const char* aTopic,
+                            const char16_t* aMessage)
+{
+  mIsMathMLDisabled = mozilla::Preferences::GetBool(kPrefMathMLDisabled);
+  mURIArray.Clear();
+  mURIToIDTable.Clear();
+  sInstance->Init();
+  return NS_OK;
Looks scary. Why can we clear the array and hashtable? Remember that there can be totally random namespaces defined by web pages and those should still work.
We need to just replace the old mathml id with new one.

Ah, in NS_NewElement you make us explicitly create normal xml elements if mathml id is used.
That should work too. I might even like it more than hacking 

In tests you can check the created element's namespaceURI in JS returns still the right value.
Please add tests also for createElement and createElementNS use.


I wonder if there is yet another option...Depends on what other browsers do and what we try to achieve here:
in HTML parser if mathml is disabled, don't do anything special with mathml elements. Just treat them as random elements.
This case is rather unclear to me.  If all the browsers create elements in MathML perhaps that shouldn't be done?
My guess this shouldn't be done, and then approach 1. is fine, assuming nsNameSpaceManager::Observe is fixed.
Flags: needinfo?(bugs)
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/2-3/
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Hey would you three be able to review this?

Notable changes from the original patch are that it still uses the MathML namespace when in JavaScript, which I use the web platform tests to check.

There is a new hash table for the disabled namespace which means SVG can use this also (this will likely require a modification to GetNamespace to allow for getting the document to permit in chrome but deny in content).
Attachment #8765923 - Flags: review?(huseby)
Attachment #8765923 - Flags: review?(bugs)
Attachment #8765923 - Flags: review?(arthuredelstein)
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/3-4/
Attachment #8765923 - Flags: review?(huseby)
Attachment #8765923 - Flags: review?(bugs)
Attachment #8765923 - Flags: review?(arthuredelstein)
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Retagging for review after I made a mistake. Let me know if you need anything.
Thanks
Attachment #8765923 - Flags: review?(huseby)
Attachment #8765923 - Flags: review?(bugs)
Attachment #8765923 - Flags: review?(arthuredelstein)
https://reviewboard.mozilla.org/r/61042/#review57962

::: dom/mathml/nsMathMLElement.cpp:112
(Diff revision 2)
>    }
>  
>    nsIDocument* doc = GetComposedDoc();
>    if (doc) {
> -    if (!doc->GetMathMLEnabled()) {
> +    if (!doc->GetMathMLEnabled() &&
> +        !Preferences::GetBool("privacy.mathml.disabled", false)) {

This has been added because the call to cache->MathMLSheet() crashes because of the null pointer returned from this method.

Perhaps there is another way?

::: dom/xml/nsXMLContentSink.cpp:1052
(Diff revision 4)
>    // elements do not get pushed on the stack, the template
>    // element content is pushed instead.
>    bool isTemplateElement = debugTagAtom == nsGkAtoms::_template &&
>                             debugNameSpaceID == kNameSpaceID_XHTML;
>    NS_ASSERTION(content->NodeInfo()->Equals(debugTagAtom, debugNameSpaceID) ||
> +               (debugNameSpaceID == kNameSpaceID_MathML && content->NodeInfo()->Equals(debugTagAtom)) ||

The normal namespace comes out of here as this appears to come direct from the parser before it has gone through the namespace manipulation.

I could potentially do this elsewhere, but I'm not sure if that makes sense. I could also make this more explicit by checking that content->NodeInfo()->NamespaceID() == kNameSpaceID_disabled_MathML.
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

https://reviewboard.mozilla.org/r/61042/#review59724

Mostly just some nits, but want to get answer to that SVG question: does tor want to be able to disable svg? And if so, we should have the same setup for mathml and svg.

::: dom/base/nsNameSpaceManager.cpp:51
(Diff revision 4)
>    }
>  
>    return sInstance;
>  }
>  
> +nsNameSpaceManager::~nsNameSpaceManager() {

nit, { goes to its own line

::: dom/base/nsNameSpaceManager.cpp:142
(Diff revision 4)
>    }
>  
>    int32_t nameSpaceID;
>  
>    nsCOMPtr<nsIAtom> atom = NS_Atomize(aURI);
> +  nsNameSpaceManager* nsmgr = nsNameSpaceManager::GetInstance();

So we won't be able to use this setup for SVG, since SVG must be enabled in chrome (UI uses it for some icons etc). I thought the idea was to pass an nsINode* or NodeInfo* as a param to this method, and then if we have some namespaces disabled, check whether we're dealing with chrome, and allow still using those namespaces normally.
XBL document, I think, should be handled as chrome.
(in practice XBL == IsLoadedAsInteractiveData())

What is the plan with SVG? Does Tor want it disabled? If so, I think we should make the setup for MathML and SVG the same already now. I mean, allow MathML usage in chrome (svg setup should be changed in a different bug.)
But if not, perhaps this patch is fine.

::: dom/base/nsNameSpaceManager.cpp:143
(Diff revision 4)
>  
>    int32_t nameSpaceID;
>  
>    nsCOMPtr<nsIAtom> atom = NS_Atomize(aURI);
> +  nsNameSpaceManager* nsmgr = nsNameSpaceManager::GetInstance();
> +  if (nsmgr &&

Why you need to access GetInstance()? This is a method of nsNamespaceManager, so just check if mMathMLDisabled is set.

::: dom/base/nsNameSpaceManager.cpp:181
(Diff revision 4)
> +    if (nsmgr && !nsmgr->mMathMLDisabled) {
> -    return NS_NewMathMLElement(aResult, ni.forget());
> +      return NS_NewMathMLElement(aResult, ni.forget());
> -  }
> +    }
> +
> +    RefPtr<mozilla::dom::NodeInfo> genericXMLNI
> +      = ni->NodeInfoManager()->

nit, put = to end of previous line

::: dom/base/nsNameSpaceManager.cpp:227
(Diff revision 4)
>    mURIToIDTable.Put(mURIArray.LastElement(), aNameSpaceID);
>  
>    return NS_OK;
>  }
> +
> +nsresult nsNameSpaceManager::AddDisabledNameSpace(already_AddRefed<nsIAtom> aURI,

nsresult to its own line and then align params if possible

::: dom/mathml/nsMathMLElement.cpp:112
(Diff revision 4)
>    }
>  
>    nsIDocument* doc = GetComposedDoc();
>    if (doc) {
> -    if (!doc->GetMathMLEnabled()) {
> +    if (!doc->GetMathMLEnabled() &&
> +        !Preferences::GetBool("privacy.mathml.disabled", false)) {

Could we check the pref value from NamespaceManager?

::: dom/svg/nsSVGFeatures.cpp:49
(Diff revision 4)
>  /*static*/ bool
>  nsSVGFeatures::HasExtension(const nsAString& aExtension)
>  {
>  #define SVG_SUPPORTED_EXTENSION(str) if (aExtension.EqualsLiteral(str)) return true;
>    SVG_SUPPORTED_EXTENSION("http://www.w3.org/1999/xhtml")
> +  if (!Preferences::GetBool("privacy.mathml.disabled", false)) {

same here.
Better to not copy-paste the pref name to too many places.

::: dom/xml/nsXMLContentSink.cpp:1052
(Diff revision 4)
>    // elements do not get pushed on the stack, the template
>    // element content is pushed instead.
>    bool isTemplateElement = debugTagAtom == nsGkAtoms::_template &&
>                             debugNameSpaceID == kNameSpaceID_XHTML;
>    NS_ASSERTION(content->NodeInfo()->Equals(debugTagAtom, debugNameSpaceID) ||
> +               (debugNameSpaceID == kNameSpaceID_MathML && content->NodeInfo()->Equals(debugTagAtom)) ||

Could you make this assertion a bit stronger, so that you check the namespaceID. Should be either real MathML or the disabled_MathML.

::: layout/mathml/tests/test_disabled.html:2
(Diff revision 4)
> +<!DOCTYPE HTML>
> +<html>

So this tests lots of other things than mathml namespace handling. I wonder why. Is this copied from some other test?

::: layout/style/nsLayoutStylesheetCache.cpp:170
(Diff revision 4)
>  {
>    return mSVGSheet;
>  }
>  
>  StyleSheetHandle
>  nsLayoutStylesheetCache::MathMLSheet()

Hmm, this method name hints this never returns null.
Do we actually need to check the pref here. The caller has it already in nsMathMLElement, no?

You could just MOZ_ASSERT here that the pref isn't false.

::: layout/style/nsLayoutStylesheetCache.cpp:172
(Diff revision 4)
>  }
>  
>  StyleSheetHandle
>  nsLayoutStylesheetCache::MathMLSheet()
>  {
> +  if (Preferences::GetBool("privacy.mathml.disabled", false)) {

this could also use the pref value from namespace manager
Attachment #8765923 - Flags: review?(bugs) → review-
Attachment #8624938 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/61042/#review59724

Yup, was going to address the method change in the SVG patch but I can push it in here.

> So we won't be able to use this setup for SVG, since SVG must be enabled in chrome (UI uses it for some icons etc). I thought the idea was to pass an nsINode* or NodeInfo* as a param to this method, and then if we have some namespaces disabled, check whether we're dealing with chrome, and allow still using those namespaces normally.
> XBL document, I think, should be handled as chrome.
> (in practice XBL == IsLoadedAsInteractiveData())
> 
> What is the plan with SVG? Does Tor want it disabled? If so, I think we should make the setup for MathML and SVG the same already now. I mean, allow MathML usage in chrome (svg setup should be changed in a different bug.)
> But if not, perhaps this patch is fine.

They do yeah so I can do the work here. I thought we could land this then amend after.

> So this tests lots of other things than mathml namespace handling. I wonder why. Is this copied from some other test?

Yup I kept them verbatim the same so it could be obvious. Will clean up.
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Removing reviewers whilst I address the other comments.
Attachment #8765923 - Flags: review?(huseby)
Attachment #8765923 - Flags: review?(arthuredelstein)
Disabling SVG via pref is bug 1216893, so yes, it is something Tor Browser wants.  The current patch used by Tor Browser to disable SVG was based on the old svg.enabled feature that was removed a while ago (with additions to allow SVG for chrome) but maybe it would be cleaner to use the disabled namespace technique.

Looking at the proposed patch (attachment 8765923 [details]), it seems like no changes should be needed inside nsMathMLElement.cpp because when MathML is disabled no nsMathMLElement objects will be created.  Or am I missing something?

Finally, while I know that huseby wants all of the Tor-related prefs to include "privacy" in their name, it seems strange to do so in this case (maybe I am just being a purist).  Disabling MathML is about security, not privacy.
nsMathMLElement will need some changes if we treat MathML the same way as SVG will be treated: chrome and XBL may create those elements, and we want to support dynamic changes to the pref.
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/4-5/
Attachment #8765923 - Flags: review-
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/5-6/
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/6-7/
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/7-8/
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Hey so I addressed the testing comments and that MathML should work in chrome context.

I actually was able to remove the svg and stylesheet cache changes with no regressions which looks like it still works in all the cases I was previously needing it which makes the overall change much simpler.

Thanks
Attachment #8765923 - Flags: review?(bugs)
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

https://reviewboard.mozilla.org/r/61042/#review61120

+the svg stuff we discussed.

::: dom/base/nsContentUtils.cpp:2891
(Diff revision 8)
>      nsAutoString nameSpace;
>      rv = aNamespaceResolver->LookupNamespaceURIInternal(Substring(aQName.get(),
>                                                                    colon),
>                                                          nameSpace);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    nsCOMPtr<nsIDocument> doc = aNamespaceResolver->NodeInfo()->GetDocument();

Please don't use nsCOMPtr here. It causes extra AddRef/Release calls.
(In general nsCOMPtr/RefPtr should be used, but here we know that the object won't die even if we don't keep strong reference.)

you could just have
  *aNamespace =
    NameSpaceManager()->GetNameSpaceID(nameSpace,
                                       aNamespaceResolver->OwnerDoc());

::: dom/base/nsDOMAttributeMap.cpp:450
(Diff revision 8)
>    int32_t nameSpaceID = kNameSpaceID_None;
>  
>    if (!aNamespaceURI.IsEmpty()) {
> +    nsCOMPtr<nsIDocument> doc = mContent->NodeInfo()->GetDocument();
>      nameSpaceID =
> -      nsContentUtils::NameSpaceManager()->GetNameSpaceID(aNamespaceURI);
> +      nsContentUtils::NameSpaceManager()->GetNameSpaceID(aNamespaceURI, doc);

similar here, just use mContent->OwnerDoc()

::: dom/base/nsNameSpaceManager.h:50
(Diff revision 8)
>    nsIAtom* NameSpaceURIAtom(int32_t aNameSpaceID) {
>      MOZ_ASSERT(aNameSpaceID > 0 && (int64_t) aNameSpaceID <= (int64_t) mURIArray.Length());
>      return mURIArray.ElementAt(aNameSpaceID - 1); // id is index + 1
>    }
>  
> -  int32_t GetNameSpaceID(const nsAString& aURI);
> +  virtual int32_t GetNameSpaceID(const nsAString& aURI,

This shouldn't be virtual

::: dom/base/nsNameSpaceManager.h:51
(Diff revision 8)
>      MOZ_ASSERT(aNameSpaceID > 0 && (int64_t) aNameSpaceID <= (int64_t) mURIArray.Length());
>      return mURIArray.ElementAt(aNameSpaceID - 1); // id is index + 1
>    }
>  
> -  int32_t GetNameSpaceID(const nsAString& aURI);
> -  int32_t GetNameSpaceID(nsIAtom* aURI);
> +  virtual int32_t GetNameSpaceID(const nsAString& aURI,
> +                                 nsCOMPtr<nsIDocument> aDocument);

parameters should be Foo*, not nsCOMPtr<Foo>, 
so, nsIDocument* aDocument

::: dom/base/nsNameSpaceManager.h:55
(Diff revision 8)
> -  int32_t GetNameSpaceID(const nsAString& aURI);
> -  int32_t GetNameSpaceID(nsIAtom* aURI);
> +  virtual int32_t GetNameSpaceID(const nsAString& aURI,
> +                                 nsCOMPtr<nsIDocument> aDocument);
> +  virtual int32_t GetNameSpaceID(nsIAtom* aURI,
> +                                 nsCOMPtr<nsIDocument> aDocument);
>  
> -  bool HasElementCreator(int32_t aNameSpaceID);
> +  virtual bool HasElementCreator(int32_t aNameSpaceID);

This shouldn't be virtual.

::: dom/base/nsNameSpaceManager.cpp:55
(Diff revision 8)
>    return sInstance;
>  }
>  
> +nsNameSpaceManager::~nsNameSpaceManager()
> +{
> +  mozilla::Preferences::RemoveObservers(this, kObservedPrefs);

So this should be a useless call. You add a strong observer, so as long as you're observing the pref changes, the object is being kept alive.

::: dom/base/nsNameSpaceManager.cpp:186
(Diff revision 8)
>    if (ns == kNameSpaceID_XUL) {
>      return NS_NewXULElement(aResult, ni.forget());
>    }
>  #endif
>    if (ns == kNameSpaceID_MathML) {
> +    nsCOMPtr<nsIDocument> doc = ni->GetDocument();

nsIDocument* doc

::: dom/base/nsNameSpaceManager.cpp:192
(Diff revision 8)
> +
> +    // If the mathml.disabled pref. is true, convert all MathML nodes into
> +    // disabled MathML nodes by swapping the namespace.
> +    nsNameSpaceManager* nsmgr = nsNameSpaceManager::GetInstance();
> +    if ((nsmgr && !nsmgr->mMathMLDisabled) ||
> +        nsContentUtils::IsChromeDoc(doc)) {

...and you could just pass the document as parameter.
nsContentUtils::IsChromeDoc(ni->GetDocument())

::: layout/mathml/tests/mochitest.ini:10
(Diff revision 8)
>  [test_bug706406.html]
>  [test_bug827713-2.html]
>  [test_bug827713.html]
>  [test_bug975681.html]
> +[test_disabled.html]
> +  prefs: [mathml.disabled:true]

Does this actually work? Don't you need to use SpecialPowers.pushPrefEnv right before running any part of the test.

::: modules/libpref/init/all.js:1177
(Diff revision 8)
>  //   2 = openAbused
>  pref("privacy.popups.disable_from_plugins", 2);
>  
>  // send "do not track" HTTP header, disabled by default
>  pref("privacy.donottrackheader.enabled",    false);
> +

Some random change here?

::: testing/web-platform/mozilla/tests/html/syntax/parsing/math-parse01.html:1
(Diff revision 8)
> +<!DOCTYPE html>

Given that there is test_disabled.html I wouldn't add this wpt test. wpt tests are something meant to be used by all the browser vendors, and this test is just a copy of some existing test.
Attachment #8765923 - Flags: review?(bugs) → review-
https://reviewboard.mozilla.org/r/61042/#review61120

> Does this actually work? Don't you need to use SpecialPowers.pushPrefEnv right before running any part of the test.

Seems to when not in a chrome.ini, can further check.

> Given that there is test_disabled.html I wouldn't add this wpt test. wpt tests are something meant to be used by all the browser vendors, and this test is just a copy of some existing test.

I spoke to jgraham about this as I wanted to just run tests to ensure it worked with a pref enabled. He suggested using the mozilla space in wpt to allow for this until we have the capability to flip a pref scenario for a certain test (however noone is actively working on this) However it does have fairly robust checking of the parsing to ensure it still works.

Happy to rip this out though.
(In reply to Jonathan Kingston [:jkt] from comment #72)
> I spoke to jgraham about this
Ok, then it is fine.
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/8-9/
Attachment #8765923 - Flags: review-
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Still waiting for try results, but should be good to go. I added a new method to check if the string is in a disabled namespace which should remove the code copy and paste for SVG.

As discussed I added the method to SVGTest to check if it is in the chrome document.

Thanks
Attachment #8765923 - Flags: review?(bugs)
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

https://reviewboard.mozilla.org/r/61042/#review61248

Those fixed, r+. Feel free to ask a new review for those small changes if needed.

::: dom/base/nsNameSpaceManager.h:55
(Diff revisions 8 - 9)
> -  virtual int32_t GetNameSpaceID(const nsAString& aURI,
> -                                 nsCOMPtr<nsIDocument> aDocument);
> -  virtual int32_t GetNameSpaceID(nsIAtom* aURI,
> -                                 nsCOMPtr<nsIDocument> aDocument);
> +  int32_t GetNameSpaceID(const nsAString& aURI,
> +                         nsIDocument* aDocument);
> +  int32_t GetNameSpaceID(nsIAtom* aURI,
> +                         nsIDocument* aDocument);
>  
> -  virtual bool HasElementCreator(int32_t aNameSpaceID);
> +  bool IsInDisabledNameSpace(const nsAString& aURI);

I think IsDisabledNameSpace() would be a tad better name (but we can just remove this)

::: dom/base/nsNameSpaceManager.h:66
(Diff revisions 8 - 9)
>  
>  private:
>    bool Init();
>    nsresult AddNameSpace(already_AddRefed<nsIAtom> aURI, const int32_t aNameSpaceID);
>    nsresult AddDisabledNameSpace(already_AddRefed<nsIAtom> aURI, const int32_t aNameSpaceID);
> -  virtual ~nsNameSpaceManager();
> +  ~nsNameSpaceManager() {};

Do we need this deconstructor at all?

::: dom/base/nsNameSpaceManager.cpp:165
(Diff revisions 8 - 9)
>    }
>  
>    return kNameSpaceID_Unknown;
>  }
>  
> +bool

ok, and based on IRC conversation this wouldn't be needed then.

::: dom/svg/SVGTests.cpp:64
(Diff revision 9)
>  SVGTests::HasExtension(const nsAString& aExtension)
>  {
> +  if (IsInChromeDoc()) {
> +    nsNameSpaceManager* nameSpaceManager = nsNameSpaceManager::GetInstance();
> +    if (nameSpaceManager->IsInDisabledNameSpace(aExtension)) {
> +      return true;

So if we happen to disable more namespaces, this starts to return true for all of them for chrome docs.

I think nsSVGFeatures::HasExtension could take a new bool param indicating the chrome-ness.
Then nsSVGFeatures::HasExtension would use that and the value of the preference.
And nsSVGFeatures::HasExtension could use the
preference value from namespacemanager, since accessing a bool member variable is way faster than calling GetBool

::: dom/svg/nsSVGFeatures.cpp:49
(Diff revision 9)
>  /*static*/ bool
>  nsSVGFeatures::HasExtension(const nsAString& aExtension)
>  {
>  #define SVG_SUPPORTED_EXTENSION(str) if (aExtension.EqualsLiteral(str)) return true;
>    SVG_SUPPORTED_EXTENSION("http://www.w3.org/1999/xhtml")
> +  if (!Preferences::GetBool("mathml.disabled", false)) {

...so this could be change a bit based on the comment above.
Attachment #8765923 - Flags: review?(bugs) → review+
https://reviewboard.mozilla.org/r/61042/#review61248

> Do we need this deconstructor at all?

Yeah else I get:
error: static assertion failed: Reference-counted class nsNameSpaceManager should not have a public destructor
Dave and Arthur, could you look over the latest patch. (I'm not sure if I can flag you for review without dropping :smaugs).

Thanks.
Flags: needinfo?(huseby)
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/9-10/
Attachment #8765923 - Flags: review+
Attachment #8765923 - Flags: review?(huseby)
Attachment #8765923 - Flags: review?(bugs)
Attachment #8765923 - Flags: review?(arthuredelstein)
Attachment #8765923 - Flags: review?(bugs) → review+
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

https://reviewboard.mozilla.org/r/61042/#review61278

::: dom/svg/nsSVGFeatures.cpp:46
(Diff revisions 9 - 10)
>  #undef SVG_UNSUPPORTED_FEATURE
>    return false;
>  }
>  
>  /*static*/ bool
> -nsSVGFeatures::HasExtension(const nsAString& aExtension)
> +nsSVGFeatures::HasExtension(const nsAString& aExtension, const bool isInChrome)

aIsInChrome

::: dom/svg/nsSVGFeatures.h:33
(Diff revision 10)
>     *
>     * @param aExtension the URI of an extension. Known extensions are
>     *   "http://www.w3.org/1999/xhtml" and "http://www.w3.org/1998/Math/MathML"
>     */
>    static bool
> -  HasExtension(const nsAString& aExtension);
> +  HasExtension(const nsAString& aExtension, const bool isInChrome);

aIsInChrome
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/10-11/
Attachment #8765923 - Attachment description: Bug 1173199 - Create preference to disable MathML. → Bug 1173199 - Create preference to disable MathML. r+smaug
Attachment #8765923 - Flags: review?(arthuredelstein)
Attachment #8765923 - Flags: review+
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/11-12/
Attachment #8765923 - Attachment description: Bug 1173199 - Create preference to disable MathML. r+smaug → Bug 1173199 - Create preference to disable MathML. r?"Arthur Edelstein", huseby, smaug
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/12-13/
Attachment #8765923 - Attachment description: Bug 1173199 - Create preference to disable MathML. r?"Arthur Edelstein", huseby, smaug → Bug 1173199 - Create preference to disable MathML.
Blocks: 1216893
Status: NEW → ASSIGNED
Flags: needinfo?(huseby)
Priority: -- → P1
Priority: P1 → P3
Hey
Flags: needinfo?(mcs)
Flags: needinfo?(brade)
Whoops, :mcs and Kathy would you be able to check this over to see if it meets your needs? The patch has changed a fair amount.
Happy top explain anything over irc.
(In reply to Jonathan Kingston [:jkt] from comment #88)
> :mcs and Kathy would you be able to check this over to see if it
> meets your needs? The patch has changed a fair amount.

The current patch should meet our needs. Thanks for all of your work!
Flags: needinfo?(mcs)
Flags: needinfo?(brade)
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

https://reviewboard.mozilla.org/r/61042/#review64826

This looks OK to me.  Thanks!
Attachment #8765923 - Flags: review?(huseby) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0bf6e0b8f1d
Create preference to disable MathML. r=huseby, r=smaug
Keywords: checkin-needed
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/13-14/
Attachment #8765923 - Flags: review?(cam)
Sorry didn't see the needinfo Wes, the new Snapshot for Servo CSS didn't implement the method that this patch uses. So I have rewritten the namespace method to accept a bool as to if it is chrome rather than the parent document itself. I think it is just a case of who's patch landed first really.

The change then adds a IsInChromeDocument(); to nsiContent which is then overridden by the servoElementSnapshot checking the bool flag which is implemented when the snapshot is created rather than checking the document.

I have flagged :heycam for review as he checked over the Stylo patches on the Gecko side in the first place.

(note: the interdiff has parts of the rebase included which is a common occurrence, look at the full diff to see the changes really)
Flags: needinfo?(jkt)
Comment on attachment 8765923 [details]
Bug 1173199 - Create preference to disable MathML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61042/diff/14-15/
Attachment #8765923 - Flags: review?(cam) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b09464eb767
Create preference to disable MathML. r=heycam, r=huseby, r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b09464eb767
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: needinfo?(arthuredelstein)
Summary: Add a pref to allow disabling MathML → Add a pref to allow disabling MathML (Tor 13548)
You need to log in before you can comment on or make changes to this bug.