Closed Bug 1216893 Opened 5 years ago Closed 3 years ago

Add pref to optionally disable SVG (Tor 12827)

Categories

(Core :: SVG, defect, P3)

Product:
Core
Component:
SVG
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: arthur, Assigned: jkt)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [tor][tor-standalone])

Attachments

(2 files)

Bug 1216893 - Add in disabled namespace for SVG
58 bytes, text/x-review-board-request
smaug
: review+
hsivonen
: review+
Details
svg rendering of documents
96.17 KB, image/png
Details 
To reduce attack surface area, in "medium-low security" mode and higher, Tor Browser disables SVG in content pages, via a boolean pref. Such a patch was recommended by a security audit.[1]  We would like to propose upstreaming this patch to Firefox. 

For reference, the original ticket is at https://trac.torproject.org/12827,
and the latest Tor Browser patch is at https://torpat.ch/2875.

[1] https://github.com/iSECPartners/publications/raw/master/reports/Tor%20Browser%20Bundle/Tor%20Browser%20Bundle%20-%20iSEC%20Deliverable%201.3.pdf#16
(In reply to Arthur Edelstein from comment #0)
> and the latest Tor Browser patch is at https://torpat.ch/2875.

Correction -- the latest Tor Browser patch is instead at https://torpat.ch/12827.
Whiteboard: [tor]
We had such a pref. We removed it because SVG is used internally in Firefox to such an extent that the browser would not start when the pref was enabled, perhaps that bug (adjusted to ignore Chrome so the browser still starts) would be a better starting point (it was smaller, bug 617448 removed it) .

You'd also need to be careful not to reintroduce bug 303581, I can't tell from inspection whether the code would do that but the bug has steps to test it.

The other problem with disabling native SVG is that you open up Firefox to use old unsupported SVG plugins such as the Adobe SVG plugin. You'd need to be sure you didn't do that.
Depends on: 1173199
Blocks: meta_tor
Priority: -- → P3
Assigning this to myself for reference as it is based on the MathML patch I wrote however I won't be working on it immediately. Feel free to ping me etc.
Assignee: nobody → jkt
Hi Both,
I have pushed a patch without any tests at the moment, however I wanted to know if tor had any patches for checking this feature.
The new patch just reuses the same disabled namespace feature I added before so should behave in the same manner there.

Thanks
Not sure why the need info wasn't added.
Flags: needinfo?(mcs)
Flags: needinfo?(brade)
isn't the tor patch listed in comment 0?
Comment 1 but it doesn't have tests.
Unfortunately, we (the Tor Project) have not developed automated tests for this.
Flags: needinfo?(mcs)
Flags: needinfo?(brade)
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

Hey Olli,
Could you review this as you took a look at the MathML patch which this extends from? (thanks for your initial help this patch is far simpler).
Thanks
Attachment #8780135 - Flags: review?(bugs)
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

Hold fire making some changes sorry.
Attachment #8780135 - Flags: review?(bugs) → review-
nsSVGFeatures::HasFeature needs to return false in all cases if SVG is disabled.
make that all non-chrome cases if SVG is disabled.
HasFeature doesn't exist in the non-chrome case due to the patch making the SVG elements not use the SVG code, is that going to be an issue?

Also to note I renamed the pref to match the mathml.disabled preference which disabled only mathml in content also.
Flags: needinfo?(longsonr)
HasFeature is the implementation of a DOM method. You don't need an SVG element to use it.

https://developer.mozilla.org/en-US/docs/Web/API/DOMImplementation/hasFeature

document.implementation.hasFeature("http://www.w3.org/TR/SVG11/feature#OpacityAttribute", "1.1");

Having said that it appears to be removed in SVG 2 so I'll raise a bug to remove it and then you won't need to worry about it here.
Flags: needinfo?(longsonr)
Depends on: 1295404
Yeah in DOM it's been deprecated too.

Sorry I meant hasExtension and I wasn't aware that was changing, this and it's predecessor bug both use hasExtension to check that the implementation is correct still: https://dxr.mozilla.org/mozilla-central/source/layout/mathml/tests/test_disabled_chrome.html We might need to find some other way of detecting namespaces are still available within SVG if that is going away right?

Thanks
Flags: needinfo?(longsonr)
Correct. Markup will still be able to detect it via a <switch> element or perhaps we could keep hasExtension but make it a chrome only method.
Flags: needinfo?(longsonr)
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

https://reviewboard.mozilla.org/r/70920/#review69458

You need to update also http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/dom/base/DOMImplementation.cpp#146 I think. Otherwise web pages can create "SVGDocuments"

Look for also other explicit "http://www.w3.org/2000/svg" string usage in C++ code.

::: dom/base/nsNameSpaceManager.cpp:158
(Diff revision 3)
>    if (aURI == nsGkAtoms::_empty) {
>      return kNameSpaceID_None; // xmlns="", see bug 75700 for details
>    }
>  
>    int32_t nameSpaceID;
> -  if (mMathMLDisabled &&
> +  if (!aInChromeDoc &&

This is too slow. You need to first check if anything is disabled, and only then do the mDisabledURIToIDTable lookup.
Attachment #8780135 - Flags: review?(bugs) → review-
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

https://reviewboard.mozilla.org/r/70920/#review69472

Also, should there be some check that when an document with SVG mime type is loaded, we treat that as a normal XML document?
Do we really need to do documents? Because of the changes the browser doesn't even render them correctly.
For example:
https://upload.wikimedia.org/wikipedia/commons/1/12/%D0%9F%D1%80%D0%B8%D0%BC%D0%B5%D1%80_%D1%87%D0%B5%D1%80%D1%82%D0%B5%D0%B6%D0%B0_%D0%B2_SVG_%D1%84%D0%BE%D1%80%D0%BC%D0%B0%D1%82%D0%B5.svg

Ends up as just text, will attach preview.
Flags: needinfo?(bugs)
Well, if the idea is to reduce attack surface area, I would expect that SVG documents weren't created.
Also, for consistency, shouldn't we just treat "create svg document" as any "create random xml document" if svg is disabled.
Flags: needinfo?(bugs)
Summary: Add pref to optionally disable SVG → Add pref to optionally disable SVG (Tor 12827)
Whiteboard: [tor] → [tor][tor-standalone]
Hey :smaug, Pretty sure I had a test for this before which I will add, I flagged you for review more if you preferred that submitted stlye of code vs producing a temporary variable like:

```
    bool isSVGPemitted = false;
    if (nsmgr && !nsmgr->mSVGDisabled) {
      isSVGPermitted = true;
    } else {
      nsCOMPtr<nsIChannel> channel = ni->GetDocument()->GetChannel();
      nsCOMPtr<nsILoadInfo> loadInfo;
      // We don't have a channel for SVGs constructed inside a SVG script
      if (channel) {
        loadInfo  = channel->GetLoadInfo();
        if (nsContentUtils::IsSystemPrincipal(loadInfo->LoadingPrincipal()) ||
         nsContentUtils::IsSystemPrincipal(loadInfo->TriggeringPrincipal())) {
          isSVGPermitted = true;
        }
      }
    }

    if (isSVGPermitted) {

```

Thanks!
Flags: needinfo?(bugs)
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

https://reviewboard.mozilla.org/r/70920/#review97630

::: dom/base/nsNameSpaceManager.cpp:220
(Diff revision 4)
> +    }
> +
> +    if ((nsmgr && !nsmgr->mSVGDisabled) ||
> +        (loadInfo &&
> +        (nsContentUtils::IsSystemPrincipal(loadInfo->LoadingPrincipal()) ||
> +         nsContentUtils::IsSystemPrincipal(loadInfo->TriggeringPrincipal())))) {

Could you explain this? Why do we care about loadingPrincipal or triggeringPrincipal, but not the actual principal of the document?

Initial reaction is that this isn't what we want, but maybe I'm missing something here.
Attachment #8780135 - Flags: review?(bugs) → review-
Hi Smaug,

I can replace one of those with the node principal (I think it was the loading principal) based on my testing however removing the other breaks most of the browser in the same way as globally disabling SVG does.

Chris I know we have discussed this on the work week and before. We need to account for SVG that are loaded within SVG, CSS and JS from System level contexts but not content ever. I actually am not able to replicate the issue I was having with cache on session restore like we also talked about.

Thanks
Jonathan
Flags: needinfo?(bugs) → needinfo?(ckerschb)
Could you still explain why use use loadingPrincipal and triggeringPrincipal? Saying "well, otherwise things don't work" isn't too clear ;)

Is there a concrete example where node principal is not system principal but
loading principal is and that case breaks FF UI?
Same with triggering principal?
The obvious breakage when this statement is just set to check the Node principal is SVG embedded within CSS.

This breaks:
- Curved tab ends, close icon and new tab
- Containers icons
- Main menu icons like preferences, developer, addons etc.
- Context menu: back, forwards, refresh, fav

However not all is broken such as:
- The branding icon in the URL bar: chrome://branding/content/identity-icons-brand.svg

This is an example of one of the icons that is broken: list-style-image: url(chrome://browser/skin/menuPanel.svg);

When I inspect this icon in the menu and hover over the element I get the following stack trace:
console.error:
  Message: [Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js :: imageToImageData< :: line 3200"  data: no]
  Stack:
    imageToImageData<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3200:5
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
Handler.prototype.process@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:932:23
this.PromiseWalker.walkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:776:7
Promise.prototype.then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:452:5
TaskImpl.prototype._handleResultValue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:387:7
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:319:13
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
exports.InspectorActor<.getImageDataFromURL@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:2764:12
generateRequestHandlers/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1057:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1752:15
DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:483:11
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14

imageToImageData<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3200:5
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
Handler.prototype.process@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:932:23
this.PromiseWalker.walkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:776:7
Promise.prototype.then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:452:5
TaskImpl.prototype._handleResultValue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:387:7
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:319:13
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
exports.InspectorActor<.getImageDataFromURL@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:2764:12
generateRequestHandlers/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1057:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1752:15
DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:483:11
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14

------


The following code makes these issues go away:
    if ((nsmgr && !nsmgr->mSVGDisabled) ||
        (loadInfo &&
         (nsContentUtils::IsSystemPrincipal(ni->GetDocument()->NodePrincipal()) ||
          nsContentUtils::IsSystemPrincipal(loadInfo->LoadingPrincipal())

The only breakage I see is in about:home the arrow doesn't load however this is content and I think should be expected.
Flags: needinfo?(bugs)
Isn't about:home running with chrome principles.

But ok, I guess I can live with LoadingPrincipal and TriggeringPrincipal checks (and then I guess also NodePrincipal check).
However, please access the nsIChannel and nsILoadInfo etc only when nsmgr->mSVGDisabled is true.
We don't want any extra refcounting or virtual calls in this possibly hot code path.
Flags: needinfo?(bugs)
One issue that I have noticed with this patch is embedded SVGs:

SVG's have a contentPolicy of nsIContentPolicy::TYPE_DOCUMENT and are currently loaded with a System TriggeringPrincipal which means they load on the page. However when I exclude these from the check also I make the page crash.

An example page is: https://www.perchandparrow.com/storage

I can't seem to work out right now why loading the faked namespace direct into a HTML document would cause this crash.


This is the statement I was working with:

    if (SVGEnabled ||
        nsContentUtils::IsSystemPrincipal(ni->GetDocument()->NodePrincipal()) ||
        (loadInfo &&
         (policyType == nsIContentPolicy::TYPE_IMAGE ||
         policyType == nsIContentPolicy::TYPE_OTHER) &&
         (nsContentUtils::IsSystemPrincipal(loadInfo->LoadingPrincipal()) ||
          nsContentUtils::IsSystemPrincipal(loadInfo->TriggeringPrincipal())
         )
        )
       ) {
The issue with the embedded SVG mentioned in #38 appears to come from the content loading:
 <svg><style>...</style>...</svg>

Changing the SVG to a blank XML element makes the parser hit assertions in parser/html/nsHtml5TreeOperation.cpp about: "Node didn't QI to style.".

Changing in the element to be a HTML node also creates issues as the namespace is wrong: http://searchfox.org/mozilla-central/source/dom/html/nsHTMLContentSink.cpp#249

Changing the element to be an SVG element with the changed namespace seems to have the desired impact I can restrict it only to this case (where nsIContentPolicy::TYPE_DOCUMENT) and perhaps file a follow up to further restrict this by changing the other assertions to pass when given a different namespace perhaps?
Flags: needinfo?(ckerschb) → needinfo?(bugs)
But if you let the element to be SVG element, then you aren't really disabling SVG. Just changing namespace.
Sounds like we need some changes to HTML parser.
hsivonen might have some ideas what to do here. I think we want parsing happen the same way even if SVG was disabled, but just need to make HTML parser to deal with cases when svg elements aren't actually created, but generic elements.
Flags: needinfo?(bugs)
Hey Henri,

Normally I like to propose solutions then ask for review however I am pretty stumped at making the parser have a benign disabled XML element that represents SVG when it is disabled. The specific case I am hitting is when the SVG element contains <style /> tags within it. I get the mentioned error in #39 "Node didn't QI to style.".

Is there any direction you think I could should take in making the parser behave the same if a generic XML element replaces an SVG node.

I would prefer to continue to use the XML node that I was generating however it triggers this assertion: http://searchfox.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#827 Should I check the namespace here or would there be a way to ignore the style code?

When generating a disabled element I am using the following code:

    RefPtr<mozilla::dom::NodeInfo> genericXMLNI =
      ni->NodeInfoManager()->
      GetNodeInfo(ni->NameAtom(), ni->GetPrefixAtom(),
        kNameSpaceID_disabled_SVG, ni->NodeType(), ni->GetExtraName());

Instead of the usual (http://searchfox.org/mozilla-central/source/dom/base/nsNameSpaceManager.cpp#200):

  return NS_NewSVGElement(aResult, ni.forget(), aFromParser);

Any pointers or suggestions would be very helpful.
Flags: needinfo?(hsivonen)
(In reply to Jonathan Kingston [:jkt] from comment #41)
> I would prefer to continue to use the XML node that I was generating however
> it triggers this assertion:
> http://searchfox.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.
> cpp#827 Should I check the namespace here or would there be a way to ignore
> the style code?

For reference, the current code is:
>      nsCOMPtr<nsIStyleSheetLinkingElement> ssle = do_QueryInterface(node);
>      NS_ASSERTION(ssle, "Node didn't QI to style.");
>      ssle->SetLineNumber(mFour.integer);

Instead of asserting that ssle is non-null, please instead check whether it is null and if it is null, assert that SVG has to be disabled.

i.e.
      nsCOMPtr<nsIStyleSheetLinkingElement> ssle = do_QueryInterface(node);
      if (ssle) {
          ssle->SetLineNumber(mFour.integer);
      } else {
          MOZ_ASSERT(prefToDisableSvgIsSet, "Node didn't QI to style, but SVG wasn't disabled.");
      }
Flags: needinfo?(hsivonen)
This is what I meant by is there a way to ignore the style blocks as I fixed the above parse errors but I get issues when the styles get attached to the <style> element itself.

The following line fails when the parser is given some stylesheet text fragment via(nsHtml5TreeOperation::AppendText):
  MOZ_ASSERT(aBuilder->IsInDocUpdate());


TextFragment Debug:
    path {
        fill: currentColor;
    }

[Child 25350] ###!!! ASSERTION: Tried to perform tree op outside update batch.: 'mFlushState == eInDocUpdate', file /home/jonathan/projects/firefox/parser/html/nsHtml5TreeOpExecutor.cpp, line 448
TextFragment Debug:

Assertion failure: aBuilder->IsInDocUpdate(), at /home/jonathan/projects/firefox/parser/html/nsHtml5TreeOperation.cpp:188
#01: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x214d818]
#02: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x214d751]
#03: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x21502a0]
#04: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x21346ae]
#05: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x2139a56]
#06: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0xdb5119]
#07: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0xe28bfc]
#08: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x15d7e6a]
#09: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x15d88b4]
#10: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cd14]
#11: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cc98]
#12: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cc5c]
#13: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x4890be8]
#14: XRE_RunAppShell[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x5a69653]
#15: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x15d8749]
#16: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cd14]
#17: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cc98]
#18: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cc5c]
#19: XRE_InitChildProcess[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x5a69052]
#20: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/plugin-container +0x4e34]
#21: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/plugin-container +0x4f05]
#22: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x20830]
#23: _start[/home/jonathan/projects/firefox/obj-devedition/dist/bin/plugin-container +0x4ce9]
#24: ??? (???:???)

Program /home/jonathan/projects/firefox/obj-devedition/dist/bin/plugin-container (pid = 25350) received signal 11.
Stack:
#01: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x7200cfd]
#02: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x759afd7]
#03: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x11390]

###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0085,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

#04: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x214d829]
#05: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x214d751]
#06: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x21502a0]
#07: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x21346ae]
#08: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x2139a56]
#09: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0xdb5119]
#10: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0xe28bfc]
#11: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x15d7e6a]
#12: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x15d88b4]
#13: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cd14]
#14: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cc98]
#15: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cc5c]
#16: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x4890be8]
#17: XRE_RunAppShell[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x5a69653]
#18: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x15d8749]
#19: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cd14]
#20: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cc98]
#21: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x153cc5c]
#22: XRE_InitChildProcess[/home/jonathan/projects/firefox/obj-devedition/dist/bin/libxul.so +0x5a69052]
#23: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/plugin-container +0x4e34]
#24: ???[/home/jonathan/projects/firefox/obj-devedition/dist/bin/plugin-container +0x4f05]
#25: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x20830]
#26: _start[/home/jonathan/projects/firefox/obj-devedition/dist/bin/plugin-container +0x4ce9]
#27: ??? (???:???)
Flags: needinfo?(hsivonen)
(In reply to Jonathan Kingston [:jkt] from comment #43)
> This is what I meant by is there a way to ignore the style blocks as I fixed
> the above parse errors 

I suggest changing nsHtml5DocumentBuilder::UpdateStyleSheet() by removing 
  nsCOMPtr<nsIStyleSheetLinkingElement> ssle(do_QueryInterface(aElement));
  NS_ASSERTION(ssle, "Node didn't QI to style.");
and adding the following to the top of the method:
  nsCOMPtr<nsIStyleSheetLinkingElement> ssle(do_QueryInterface(aElement));
  if (!ssle) {
    MOZ_ASSERT(prefToDisableSvgIsSet, "Node didn't QI to style, but SVG wasn't disabled.");
    return;
  }
Flags: needinfo?(hsivonen)
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

https://reviewboard.mozilla.org/r/70920/#review102556

::: dom/base/nsNameSpaceManager.cpp:195
(Diff revisions 4 - 5)
>    if (ns == kNameSpaceID_MathML) {
>      // 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(ni->GetDocument())) {
> +        nsContentUtils::IsSystemPrincipal(ni->GetDocument()->NodePrincipal())) {

Why this change? IsChromeDoc() does anyhow principal check. To be consistent with the svg checks below?

::: dom/base/nsNameSpaceManager.cpp:224
(Diff revisions 4 - 5)
> -    }
> +      }
> -
> -    if ((nsmgr && !nsmgr->mSVGDisabled) ||
> +    }
> +    if (SVGEnabled ||
> +        nsContentUtils::IsSystemPrincipal(ni->GetDocument()->NodePrincipal()) ||
>          (loadInfo &&
> +         (loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_IMAGE ||

aha, we want to keep svg loaded as images working?

::: parser/html/nsHtml5DocumentBuilder.h:108
(Diff revision 5)
>  
>    // nsContentSink methods
>    virtual void UpdateChildCounts() override;
>    virtual nsresult FlushTags() override;
>  
> +  nsNameSpaceManager* mNameSpaceManager;

Why you need this member variable?
nsNameSpaceManager is a singleton, so you should be able to access it using GetInstance

::: parser/html/nsHtml5TreeOperation.h:494
(Diff revision 5)
>      }
>  
>      nsresult Perform(nsHtml5TreeOpExecutor* aBuilder,
>                       nsIContent** aScriptElement);
>  
> +    nsNameSpaceManager* mNameSpaceManager;

Also here. Please don't add mNameSpaceManager
Attachment #8780135 - Flags: review?(bugs) → review+
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

https://reviewboard.mozilla.org/r/70920/#review102556

> Why this change? IsChromeDoc() does anyhow principal check. To be consistent with the svg checks below?

Yeah just for consistency.

> aha, we want to keep svg loaded as images working?

Only when the loading or triggering principal is system. This check is actually to prevent top level documents loading as svg because they have a system principal.
I had tests that I removed as they were broken and I can't find the patch.

However the following should be added before this is landed
- A test to compare content vs chome height rendered with the pref disabled. We have a test for this for MathML
- A test checking that embedded <svg><style /></svg> in HTML doesn't crash when the pref is enabled.

The latest patch removes the mNameSpaceManager code.
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

https://reviewboard.mozilla.org/r/70920/#review102814

::: dom/base/nsNameSpaceManager.cpp:302
(Diff revision 6)
>  NS_IMETHODIMP
>  nsNameSpaceManager::Observe(nsISupports* aObject, const char* aTopic,
>                              const char16_t* aMessage)
>  {
>    mMathMLDisabled = mozilla::Preferences::GetBool(kPrefMathMLDisabled);
> +  mSVGDisabled = mozilla::Preferences::GetBool(kPrefSVGDisabled);

Is observing these actually safe as opposed to reading these on startup and keeping the same value for the entire lifetime of the process? That is, have you tested that Gecko doesn't get too badly confused if the pref is flipped after a page has been loaded in a particular state and the JS on the page continue to run after the pref flip?

::: layout/svg/moz.build:17
(Diff revision 6)
> +        'tests/mochitest.ini',
> +]
> +    MOCHITEST_CHROME_MANIFESTS += [
> +        'tests/chrome.ini',
> +]
> +

I assume that what looks like incompleteness here is going to be addressed per https://bugzilla.mozilla.org/show_bug.cgi?id=1216893#c49

::: parser/html/nsHtml5TreeBuilderCppSupplement.h:80
(Diff revision 6)
>    NS_PRECONDITION(aAttributes, "Got null attributes.");
>    NS_PRECONDITION(aName, "Got null name.");
>    NS_PRECONDITION(aNamespace == kNameSpaceID_XHTML || 
>                    aNamespace == kNameSpaceID_SVG || 
> +                  aNamespace == kNameSpaceID_disabled_SVG ||
>                    aNamespace == kNameSpaceID_MathML,

Not strictly about this bug, but I'm wondering why kNameSpaceID_disabled_MathML does not show up here, too.
Attachment #8780135 - Flags: review?(hsivonen) → review+
Comment on attachment 8780135 [details]
Bug 1216893 - Add in disabled namespace for SVG

https://reviewboard.mozilla.org/r/70920/#review102814

> Is observing these actually safe as opposed to reading these on startup and keeping the same value for the entire lifetime of the process? That is, have you tested that Gecko doesn't get too badly confused if the pref is flipped after a page has been loaded in a particular state and the JS on the page continue to run after the pref flip?

It doesn't get badly confused, refreshing a page hard and changing a pref mid page load actually makes some of the SVGs load and some not on the page. This to me would be expected behaviour, I wanted to make the code not require a browser reload.

> I assume that what looks like incompleteness here is going to be addressed per https://bugzilla.mozilla.org/show_bug.cgi?id=1216893#c49

Yup working on this now.

> Not strictly about this bug, but I'm wondering why kNameSpaceID_disabled_MathML does not show up here, too.

I don't think either are needed actually, checking a debug build to see if it breaks tests.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0d48c28cc547
Add in disabled namespace for SVG r=hsivonen,smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d48c28cc547
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1330294
Depends on: 1330675
Depends on: 1564208
You need to log in before you can comment on or make changes to this bug.