1269885 - Set "none" flag on nsSVGViewBox base value, when we (re)initialize it

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

As discussed in bug 1269174 comment 11, I think it'd be worth setting the nsSVGViewBox::none boolean to "true" (as in "this value represents 'none'") in the Init() method.

We shouldn't actually use mBaseValue for rendering after Init() has been called (until a new attribute value has been set/parsed), so this technically shouldn't matter. But in the off chance that we do use this value somehow (as we did in bug 1269174), it would be better if we treated it as representing "none", the default behavior.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Robert, what do you think about taking this patch?  This shouldn't be strictly necessary, but it's also pretty cheap, and it would've saved us from bug 1269174. (On the other hand, it also would have prevented us from noticing bug 1269174, which kind of argues against taking it... Two sides of the same coin, I guess.)

Comment 2

[Daniel Holbert [:dholbert]]

> it also would have prevented us from noticing
> bug 1269174, which kind of argues against taking it...

(Looking at this more charitably: it would have made bug 1269174 a non-issue.)

Comment 3

[Robert Longson [:longsonr]]

the none flag indicates that viewBox="none" I don't think we should change this meaning.

Comment 4

[Robert Longson [:longsonr]]

https://www.w3.org/TR/SVGTiny12/coords.html#ViewBoxAttribute

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #3)
> the none flag indicates that viewBox="none"

Right! That's the point. :)

viewBox="none" *is* the lacuna value of this attribute (the value that is represented when the attribute is unset), as indicated by the spec text that you linked to.

> I don't think we should change
> this meaning.

I'm not suggesting that we change any meaning. It sounds like you think I'm interpreting "none" as "no attribute", but I'm not.

The point here is: if we happen to *accidentally* use mBaseVal before it's had its value populated, or after it's been invalidated by an attribute-clearing setAttribute call, it'd be better for us to treat it as if it had the value "none" (since that's what our viewBox does in fact represent, until it's been set to an actual value, because "none" is the lacuna value).

So: this is basically just making our Init() function a bit more comprehensive.

(Tagging you as needinfo to see if any of the above changes your views on the patch. I am OK not taking it, since it should strictly be unnecessary; I just don't want to not-take-it based on a misunderstanding. :))

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 8748401 [details] [diff] [review]
fix v1

Changing my mind based on Daniel's "this is the lacuna" value argument.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8748401 [details] [diff] [review]
fix v1

Review of attachment 8748401 [details] [diff] [review]:
-----------------------------------------------------------------

OK, thanks! Let's make it official with the r? flag, then. :)

Comment 8

[Robert Longson [:longsonr]]

Overall, I think it would be better to set none in the nsSVGViewBoxRect constructor.

Comment 9

[Daniel Holbert [:dholbert]]

We already do that, as it turns out:
> 34   nsSVGViewBoxRect() : none(true) {}
http://mxr.mozilla.org/mozilla-central/source/dom/svg/nsSVGViewBox.h#34

(I hadn't actually realized this -- this makes me a bit less concerned about this bug, since it at least means we don't have to worry about uninitialized-data-reads -- we'd only have to worry about potentially-stale viewBox values.)

The nsSVGViewBoxRect constructor doesn't help for the scenario that I care about, though -- when someone does setAttribute("viewBox", "") and we neglect to check the "mHasBaseVal" flag.  (as in bug 1269174).

Anyway -- I think I'll just call this WONTFIX, because it's probably unnecessary anyway.

Comment 10

[Robert Longson [:longsonr]]

I do think this is worth landing as is. Init can be called multiple times, every time we set a value to something that's invalid for instance. The constructor is only called once. 

So "0 0 100 100" to "none" to "" would leave none in a different state than "0 0 100 100" to "" which seems undesirable.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #10)
> I do think this is worth landing as is. Init can be called multiple times,
> every time we set a value to something that's invalid for instance.

Yeah, that's the scenario I was describing at the end of comment 9 (which we hit in bug 1269174).

> So "0 0 100 100" to "none" to "" would leave none in a different state than
> "0 0 100 100" to "" which seems undesirable.

Technically the "different state" shouldn't matter, since the state in question is in somewhat-invalid memory (in the sense that we're kinda not supposed to read any of these member-variables unless mHasBaseVal is true).

But I agree that we might as well take this, since it's pretty cheap and it makes this code more robust. I'll go ahead and land.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5785ece1f2
Enforce that a (re)initialized nsSVGViewBox's mBaseValue represents "none". r=longsonr

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f5785ece1f2