Opened 3 years ago

Closed 2 years ago

#13548 closed enhancement (fixed)

Create preference to disable MathML

Reported by: gk Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-security, firefox-patch, tbb-4.5-alpha, TorBrowserTeam201503, TorBrowserTeam201503R, GeorgKoppen201503R
Cc: arthuredelstein, boklm, mikeperry Actual Points:
Parent ID: #9387 Points:
Reviewer: Sponsor:

Description

We should have a way to disable MathML support in Firefox for the security slider. There currently is no pref in Firefox for this, so we will need to create one.

Child Tickets

Change History (19)

comment:1 Changed 3 years ago by arthuredelstein

Cc: arthuredelstein added

comment:2 Changed 3 years ago by boklm

Cc: boklm added

comment:3 Changed 3 years ago by gk

Keywords: tbb-4.5-alpha added

comment:4 Changed 3 years ago by mcs

Kathy and I spent some time looking at this over the past couple of days. We have not assigned the bug to one of us yet because we have not gotten very far. There are a couple of approaches we are looking at:

  1. Remove knowledge of <math> and related tags from the parser. This will work with limitations, but it may be difficult to access prefs. deep inside the parser (e.g., the code is maintained in Java and translated to C++). Also, the parser is not invoked when JS is used to add elements, so we would need a different solution for that case.
  2. Modify NS_NewElement() to treat <math> and related tags as unknown/generic tags instead of MathML tags. This seems like a good solution, but so far we have not been able to get it to work (other code still recognizes the elements as MathML).
  3. Disable special behavior / rendering / etc. for MathML tags. We have not tried this yet, but it might be do-able via modifications to content/mathml/content/src/nsMathMLElement.cpp.

We will continue to work on this; if anyone else has an idea for how to cleanly do this kind of thing inside Firefox, please comment here.

comment:5 in reply to:  4 Changed 3 years ago by mcs

Replying to mcs:

  1. Modify NS_NewElement() to treat <math> and related tags as unknown/generic tags instead of MathML tags. This seems like a good solution, but so far we have not been able to get it to work (other code still recognizes the elements as MathML).

A quick update: we got the above approach to work. The trick we are using is to replace the namespace on <math> and related elements with the generic XML namespace. We will tie this to a pref. now.

comment:6 Changed 3 years ago by mcs

Owner: changed from tbb-team to mcs
Status: newassigned

comment:7 Changed 3 years ago by mcs

Keywords: firefox-patch TorBrowserTeam201503R added
Status: assignedneeds_review

Our proposed fix is here:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug13548-01&id=5b48b325cf54fe341c9c6c417ecfbb83cc5625be
(on the bug13548-01 branch within the user/brade/tor-browser.git repo).

Please review for code correctness as well as approach.

comment:8 Changed 3 years ago by mcs

Cc: mikeperry added

comment:9 Changed 3 years ago by mcs

I already found a disadvantage of this approach: some pages have CSS that tries to make up for a lack of MathML support, but the CSS will not work if it relies on the MathML XML namespace (which our patch conditionally removes). See for example: https://developer.mozilla.org/en-US/docs/Web/MathML/Examples/Deriving_the_Quadratic_Formula

I am not sure if this is a fatal flaw or not, but it is disappointing.

comment:10 Changed 3 years ago by gk

What speaks against the "just don't render these tags" (should we make a non-chrome/chrome distinction?), directly? That would give us what we want while not breaking the CSS "fallback". It is option three from above if I understood it correctly, I think.

Last edited 3 years ago by gk (previous) (diff)

comment:11 in reply to:  10 Changed 3 years ago by brade

Replying to gk:

What speaks against the "just don't render these tags" (should we make a non-chrome/chrome distinction?), directly? That would give us what we want while not breaking the CSS "fallback". It is option three from above if I understood it correctly, I think.

Nothing speaks against it except we did not figure out how to do that yet. But we will take a look.

comment:12 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201503 added; TorBrowserTeam201503R removed
Status: needs_reviewneeds_revision

comment:13 Changed 3 years ago by mcs

It turns out that approach 3 (disable special behavior / rendering / etc. for MathML tags) is messy. A lot of special behavior is based on the XML namespace, so removing that (as the existing patch does) made a lot of things simpler. Kathy and I are still looking, but fort approach 3 so far we have:

  • In the following two files, we would need to ensure that the mathml.css stylesheet is not added to the document:
    • content/mathml/content/src/nsMathMLElement.cpp
    • parser/htmlparser/src/nsExpatDriver.cpp
  • In layout/base/nsCSSFrameConstructor.cpp we would need to avoid calling FindMathMLData(). We would also need to add "if (prefIsDisabled)" checks in about 6 other places where special behavior is invoked in response to the presence of a MathML element (look for kNameSpaceID_MathML in that file).
  • In browser/base/content/nsContextMenu.js, we would need to make changes to ensure that the "View MathML Source" context menu item is not shown when MathML is disabled.
  • We may need to make changes to content/base/src/nsTreeSanitizer.cpp, although it is probably OK to allow the math tags through even if MathML is disabled. I am not sure exactly when nsTreeSanitizer is used; maybe only when displaying RSS/Atom feeds.

In summary, a lot of changes are required, which will lead to less confidence that we have truly disabled all of the MathML functionality (as well as a somewhat complex patch to maintain).

Opinions? The "swap the namespace" patch that we already posted is a lot simpler and we are confident that it disables MathML. On the other hand, because the MathML namespace is lost, it breaks some assumptions made by web pages.

For the record, Kathy is firmly in the "let's go with the existing namespace patch" and I am undecided. But I too do not like the extra complexity of approach 3, and MathML is probably uncommon enough that no one will complain about the namespace issue.

comment:14 Changed 3 years ago by mcs

Status: needs_revisionneeds_information

comment:15 in reply to:  13 ; Changed 3 years ago by gk

Keywords: TorBrowserTeam201503R added
Status: needs_informationneeds_review

Replying to mcs:

Opinions? The "swap the namespace" patch that we already posted is a lot simpler and we are confident that it disables MathML. On the other hand, because the MathML namespace is lost, it breaks some assumptions made by web pages.

For the record, Kathy is firmly in the "let's go with the existing namespace patch" and I am undecided. But I too do not like the extra complexity of approach 3, and MathML is probably uncommon enough that no one will complain about the namespace issue.

Let's try the patch you already have then and see what breaks. Especially given the time constraints we have for the release and keeping in mind that there is #12827 waiting as well. Putting it back to needs_review again.

comment:16 in reply to:  15 Changed 3 years ago by brade

Replying to gk:

Let's try the patch you already have then and see what breaks. Especially given the time constraints we have for the release and keeping in mind that there is #12827 waiting as well. Putting it back to needs_review again.

I think taking the current patch and seeing what breaks is a good approach. I am open to revisiting the issue after Firefox 38.

comment:17 Changed 3 years ago by gk

Keywords: GeorgKoppen201503R added

comment:18 Changed 3 years ago by mikeperry

gk - Based just on code review, this patch looks OK to me. I haven't merged it yet in case you wanted to do some other investigation/testing/review first. Feel free to merge it if/when you're happy with it.

comment:19 Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looked good to me, too. There was just an the pref missing in 000-tor-browser.js. I added that one. Commit 04e603a60a13aed5434a6d5450b432a8f5ab7c46 and 1ca437755964660f39630f82cc1305670fef7874 have the changes.

Note: See TracTickets for help on using tickets.