Closed
Bug 1216893
Opened 5 years ago
Closed 4 years ago
Add pref to optionally disable SVG (Tor 12827)
Categories
(Core :: SVG, defect, P3)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla53
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)
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
Reporter | ||
Comment 1•5 years ago
|
||
(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.
Reporter | ||
Updated•5 years ago
|
Whiteboard: [tor]
Comment 2•5 years ago
|
||
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.
Updated•5 years ago
|
Priority: -- → P3
Assignee | ||
Comment 3•5 years ago
|
||
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
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61594da93eb6
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•5 years ago
|
||
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
Assignee | ||
Comment 7•5 years ago
|
||
Not sure why the need info wasn't added.
Flags: needinfo?(mcs)
Flags: needinfo?(brade)
Comment 10•5 years ago
|
||
Unfortunately, we (the Tor Project) have not developed automated tests for this.
Flags: needinfo?(mcs)
Flags: needinfo?(brade)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•5 years ago
|
||
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)
Assignee | ||
Comment 13•5 years ago
|
||
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-
Comment 14•5 years ago
|
||
nsSVGFeatures::HasFeature needs to return false in all cases if SVG is disabled.
Comment 15•5 years ago
|
||
make that all non-chrome cases if SVG is disabled.
Assignee | ||
Comment 16•5 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 18•5 years ago
|
||
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)
Assignee | ||
Comment 19•5 years ago
|
||
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)
Comment 20•5 years ago
|
||
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 21•5 years ago
|
||
mozreview-review |
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 22•5 years ago
|
||
mozreview-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?
Assignee | ||
Comment 23•5 years ago
|
||
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)
Assignee | ||
Comment 24•5 years ago
|
||
Comment 25•5 years ago
|
||
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)
Reporter | ||
Updated•5 years ago
|
Summary: Add pref to optionally disable SVG → Add pref to optionally disable SVG (Tor 12827)
Updated•5 years ago
|
Whiteboard: [tor] → [tor][tor-standalone]
Assignee | ||
Comment 26•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0f0c005a772
Assignee | ||
Comment 27•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f878ce1f971
Assignee | ||
Comment 28•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f5dcf1a5593
Assignee | ||
Comment 29•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce91f424de2d
Assignee | ||
Comment 30•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fd25f50e61c
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•4 years ago
|
||
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 33•4 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 34•4 years ago
|
||
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)
Comment 35•4 years ago
|
||
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?
Assignee | ||
Comment 36•4 years ago
|
||
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)
Comment 37•4 years ago
|
||
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)
Assignee | ||
Comment 38•4 years ago
|
||
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()) ) ) ) {
Assignee | ||
Comment 39•4 years ago
|
||
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)
Comment 40•4 years ago
|
||
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)
Assignee | ||
Comment 41•4 years ago
|
||
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)
Assignee | ||
Comment 43•4 years ago
|
||
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 hidden (mozreview-request) |
Comment 46•4 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 47•4 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•4 years ago
|
||
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 50•4 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 51•4 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 52•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=afe4ad20d75c
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Keywords: checkin-needed
Comment 54•4 years ago
|
||
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
Comment 55•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0d48c28cc547
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•4 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•