When XSLT landed, we had a preference to disable it in case a major security hole was found. Might be usefull for SVG when it gets build by default for the first major milestone that uses it (1.8, 1.9, whatever).

Taking and increasing severity since this blocks SVG from being turned on by default.

Okay, this is my work in progress. Stopping SVG documents from being created isn't quite good enough yet. If the pref is set to turn SVG off, I get a warning and two assertions as a result of the changes to nsContentDLF.cpp. WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(docLoaderFactory->CreateInstance("view", aOpenedChannel, aLoadGroup, aContentType, static_cast< nsIContentViewerContainer * >(this), 0, aContentHandler, aViewer))) failed, file c:/mozilla/trees/trunk2/mozilla/docshell/base/nsDocShell.cpp, line 4821 ###!!! ASSERTION: DoContent returned no listener?: 'abort || m_targetStreamListener', file c:/mozilla/trees/trunk2/mozilla/uriloader/base/nsURILoader.cpp, line 755 ###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error', file c:/mozilla/trees/trunk2/mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 459 Error loading URL http://jwatt.org/moz/circles1.svg : 8000ffff (NS_ERROR_UNEXPECTED) http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#4856 http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsURILoader.cpp#755 http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsInputStreamPump.cpp#459 If I move the check from nsContentDLF.cpp to NS_NewSVGDocument and have that function pass back a null pointer and return NS_ERROR_FAILURE I get the exact same warning and assertions. I haven't even checked to see if having the pref turned off will cause mozilla to look for and render SVG using a plug-in. For now I need some sleep. I plan to continue with this tomorrow, but in the mean time comments would be welcome. :-)

Those assertions happen because you register a content viewer for that type without checking the pref....

I see, thanks. Unfortunately the code isn't written to allow types to be dynamically registered/unregistered, and I'll need to register/unregister the type in my pref observer. Is it okay to expose RegisterTypes and UnregisterTypes in nsContentDLF.cpp to other files?

Wouldn't it be better to put the SVG_CheckMasterSVGPref check inside NS_NewSVGElement and NS_NewSVGDocument functions instead. That way the code wouldn't be as spread out and the risk tha a creationpoint was missed would be smaller.

I could do, but if I add something like #ifdef MOZ_SVG // methods to facilitate turning SVG support on/off dynamically static NS_IMETHODIMP RegisterSVG(); static NS_IMETHODIMP UnregisterSVG(); #endif to the declaration of nsContentDLF, I'll be touching nsContentDLF.h and nsContentDLF.cpp anyway. By doing the checks in nsContentDLF.cpp instead of nsSVGDocument.cpp the check gets done a bit earlier, and I touch fewer files.

IMHO the most important part is that the code is easy and simple to follow, as well as secure aginst bugs (the former helps the latter). Not how many files you touch or how early the break is made. Espcially this isn't performance critical

You're right. I'll move the check to NS_NewSVGDocument.

Are we actually trying to make this a pref that users can toggle at runtime? Not just one that takes effect after restart?

That's what I'm trying to do, yes. Do you think I shouldn't?

I don't really have a strong opinion, but what happens to SVG documents that are already loaded when the pref is toggled? To scripts running in them? What about SVG documents that are in the middle of being parsed?

I only plan to prevent the creation of new SVG documents and elements. Documents that are already loaded when the pref is toggled will be left in place. Scripts that are running in such documents will break if they try to create new SVG documents or elements. SVG documents that are in the middle of being parsed will fail to create any remaining elements.

Using nsContentUtils::RegisterPrefCallback might simplify the code.

The pref should also be added to modules/libpref/src/init/all.js.

jwatt: I would suggest for possibly a different bug that any and all scripts SVG related get flushed and the SVG content gets replaced with a static "SVG Not Supported" (or some sort of reload on the page as if it was normal initially). Something other than just letting stuff break. This of course if dynamic support is the final intent. I can see arguements for "This works dynamically but really should be a restart" since we will want a plugin SVG viewer to work when this pref is off if I remember initial "thoughts" on the pref thing right. /me wonders if he went on too much of a tangent, if so ignore me

Requiring a restart is fine(In reply to comment #12) > I only plan to prevent the creation of new SVG documents and elements. Documents > that are already loaded when the pref is toggled will be left in place. Scripts > that are running in such documents will break if they try to create new SVG > documents or elements. SVG documents that are in the middle of being parsed will > fail to create any remaining elements. I'd have no problems with it only working on a restart too :)

Another attempt, just trying for a restart style pref. Not sure how to conditionally register the content handlers - what I have now seems rather gross and doesn't work. Element factory untouched until I have basic document stuff working.

Comment on attachment 173662 [details] [diff] [review] working live pref Thanks for taking this tor! I wouldn't have managed to figure out the rest before the freeze. I do see one little error in that patch though. In NS_NewSVGElement you forgot the call parentheses, so it should be if (!SVGEnabled()) return NS_NewXMLElement(aResult, aNodeInfo);

Comment on attachment 173694 [details] [diff] [review] fix mistake pointed out by jwatt >Index: modules/libpref/src/init/all.js >+pref("svg.enabled", false); Don't we want to default to true? I think we do... Otherwise all existing SVG builds will suddenly stop working and people will be confused. ;) >Index: layout/build/nsLayoutModule.cpp >+PRBool SVGEnabled(); That should be extern, no? >Index: content/svg/content/src/nsSVGElementFactory.cpp >+SVGEnabled() >+ gSVGEnabled = nsContentUtils::GetBoolPref(SVG_PREF_STR); Pass PR_TRUE for the default? I think we want SVG on if the pref is not set. >Index: uriloader/base/Makefile.in I'm not particularly happy with SVG code in uriloader.... wouldn't it make more sense for the SVG content handlers to just refuse to do anything if SVG is disabled? That is, just return NS_ERROR_WONT_HANDLE_CONTENT?

(In reply to comment #21) > (From update of attachment 173694 [details] [diff] [review] [edit]) > >Index: modules/libpref/src/init/all.js > >+pref("svg.enabled", false); > > Don't we want to default to true? I think we do... Otherwise all existing SVG > builds will suddenly stop working and people will be confused. ;) Yes, but drivers currently want svg disabled by default in the 1.8 timeframe, so I constructed the patch that way. > >Index: layout/build/nsLayoutModule.cpp > >+PRBool SVGEnabled(); > > That should be extern, no? Don't think that's necessary - my reading of the C specification is that external linkage is the default for function declarations. > >Index: content/svg/content/src/nsSVGElementFactory.cpp > >+SVGEnabled() > >+ gSVGEnabled = nsContentUtils::GetBoolPref(SVG_PREF_STR); > > Pass PR_TRUE for the default? I think we want SVG on if the pref is not set. See above for which way we're defaulting. > >Index: uriloader/base/Makefile.in > > I'm not particularly happy with SVG code in uriloader.... wouldn't it make more > sense for the SVG content handlers to just refuse to do anything if SVG is > disabled? That is, just return NS_ERROR_WONT_HANDLE_CONTENT? I'll experiment with that tomorrow, but I think that might be too late to make the loading system fall back to using the plugin (if available).

Drivers want SVG off in builds that explicitly --enable-svg, by default? Or they want the --enable-svg option off by default? The latter I believe; the former seems odd (pay the space cost for the code, but have it off). > but I think that might be too late to make the loading system fall back to > using the plugin The plugin, if it exists, will get picked up in step #1 in the URILoader for the normal case (clicks on links, iframes, etc). You're changing step #4.... That code will only get hit if we have no way to handle this internally and have to decide on content handler vs external helper app.

(In reply to comment #23) > Drivers want SVG off in builds that explicitly --enable-svg, by default? Or > they want the --enable-svg option off by default? The latter I believe; the > former seems odd (pay the space cost for the code, but have it off). It's the former. > > but I think that might be too late to make the loading system fall back to > > using the plugin > > The plugin, if it exists, will get picked up in step #1 in the URILoader for the > normal case (clicks on links, iframes, etc). You're changing step #4.... That > code will only get hit if we have no way to handle this internally and have to > decide on content handler vs external helper app. Step #4 seemed to be where things went horribly wrong when the categories were gone but the content handlers were still around. Aren't the plugins part of step #6 (helper apps)?

no, plugins are not helper apps. helper apps are always executables launched as a separate process. plugins are gecko-content-viewers... but they don't overwrite built-in viewers, I believe. so maybe changing the SVG pref needs to trigger plugin re-registration?

Hmm, I wonder if people who build --enable-svg after this patch will be surprised by the sudden need to set a pref to make SVG work even after they need to --enable it. Which is why, imho, we should pass true for now, and then change the pref to false once we patch to make --enable be --disable (for SVG to be built by default). People who currently build --enable-svg will want it on, people who do after this patch, _because of_ this patch will change the pref, and the rest of the people, won't be affected until after it is built by default, no?

Comment on attachment 173798 [details] [diff] [review] remove uriloader change - require bug 250936 fix > Don't think that's necessary - my reading of the C specification is > that external linkage is the default for function declarations. Yes, but why bother depending on what compilers happen to implement when we can avoid all that _and_ increase code clarity? In fact, please add a comment saying where this function is actually defined. r=bzbarsky with that.

Comment on attachment 173798 [details] [diff] [review] remove uriloader change - require bug 250936 fix In SVGPrefChanged, add a check that the value has actually changed. Because in some situations we seem to be able to get a callback even when the value hasn't changed (e.g., when the user value is cleared but the user value is the same as the default value).

Adjust per bz and roc's comments, plus: Add SVGEnabled() checks to nsContentDLF::CreateInstance so that we won't create a SVG document before the plugin can get a chance (pref off case). Add SVGEnabled() checks to nsCSSFrameConstructor so that we won't try creating SVG frame atop generic XML nodes when dealing with compound documents.

Comment on attachment 174622 [details] [diff] [review] updated patch r=bzbarsky

Check in, information about the preference posted to mozilla.svg newsgroup and the mozilla svg update weblog.

Is there some website where we can test (and oogle over) this new SVG fuctionality? :-)

SVG is not built in the standard configuration yet - you'll still need explicitly enable it in your builds or download ones with svg enabled.

I thought "checked-in"(1) meant they would soon (tomorrow?) be in the trunk nightlies. I knew about "about:config | svg.enabled = true" from your blog(1). (1) http://weblogs.mozillazine.org/tor/archives/2005/02/important_svg_b.html

tor, with a build post this checkin, with the pref disabled, I don't get a helper app dialog when I open up an svg file; I just get a Error loading URL file:///home/bzbarsky/test.svg : 8000ffff (NS_ERROR_UNEXPECTED) in the xterm where I launched Mozilla. Any idea what's going on there? Can you reproduce that?

Comment on attachment 175895 [details] [diff] [review] keep svg mimetypes out of persistent repository r+sr=bzbarsky