Closed Bug 1269885 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

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.
Attached patch fix v1Splinter Review
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.)
Attachment #8748401 - Flags: feedback?(longsonr)
> 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.)
Summary: Enable nsSVGViewBox "none" flag, when we reinitialize it → Set "none" flag on nsSVGViewBox base value, when we (re)initialize it
the none flag indicates that viewBox="none" I don't think we should change this meaning.
Attachment #8748401 - Flags: feedback?(longsonr) → feedback-
(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. :))
Flags: needinfo?(longsonr)
Comment on attachment 8748401 [details] [diff] [review]
fix v1

Changing my mind based on Daniel's "this is the lacuna" value argument.
Flags: needinfo?(longsonr)
Attachment #8748401 - Flags: feedback- → feedback+
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. :)
Attachment #8748401 - Flags: review?(longsonr)
Overall, I think it would be better to set none in the nsSVGViewBoxRect constructor.
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Attachment #8748401 - Flags: review?(longsonr) → review+
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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.
Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5785ece1f2
Enforce that a (re)initialized nsSVGViewBox's mBaseValue represents "none". r=longsonr
https://hg.mozilla.org/mozilla-central/rev/9f5785ece1f2
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.