Closed Bug 1125647 Opened 5 years ago Closed 5 years ago

Mark methods and attributes pure and const where appropriate

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(3 files, 2 obsolete files)

No description provided.
Attached patch const.txt (obsolete) — Splinter Review
About half of Animated things are marked const. This is the other half that aren't. Bonus points if you stay awake.
Attachment #8554298 - Flags: review?(bzbarsky)
Attached patch Stuff I think is [pure] (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8554299 - Flags: review?(bzbarsky)
Summary: Mark methods pure and const where appropriate → Mark methods and attributes pure and const where appropriate
Comment on attachment 8554298 [details] [diff] [review]
const.txt

I'd appreciate it if you could get someone who actually knows the SVG code and can tell whether these things are really constant to review this.  I can't tell offhand without more digging into what things like "mEnumAttributes[CLIPPATHUNITS].ToDOMAnimatedEnum(this)" do....
Attachment #8554298 - Flags: review?(bzbarsky)
Comment on attachment 8554299 [details] [diff] [review]
Stuff I think is [pure]

SVG*List.numberOfItems/getItem/length don't look pure to me.  They presumably have side-effects via Element()->FlushAnimations(), no?

The PathSeg and SVGDocument bits look ok.
Attachment #8554299 - Flags: review?(bzbarsky) → review-
Attachment #8554298 - Flags: review?(dholbert)
Attachment #8554299 - Attachment is obsolete: true
Attachment #8554727 - Flags: review?(bzbarsky)
Attachment #8554727 - Attachment is patch: true
Maybe I need to fix bug 839711 first for the Constant patch.
Comment on attachment 8554298 [details] [diff] [review]
const.txt

I haven't really touched much webidl before. What are the implications of adding these [Constant] annotations?  (And what happens if we annotate something as [Constant] incorrectly?)

https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Constant says:
  "This should only be used when it's absolutely guaranteed that the return value of the attribute getter will always be the same from the JS engine's point of view."

I expect that should be true of all these functions, though I haven't gone through them one-by-one (ah, and as you say, bug 839711 indicates it doesn't hold up 100%).  I'm also not clear on what badness would happen if we violated MDN's "should" requirement somewhere.  I want to understand that better before granting r+ -- can you elaborate on that?  (And do we have any sorts of test/compiler guarantees that we're living up to that requirement?)

>+++ b/dom/webidl/SVGTests.webidl
> interface SVGTests {
> 
>+  [Constant]
>   readonly attribute SVGStringList requiredFeatures;
>+  [Constant]
>   readonly attribute SVGStringList requiredExtensions;
>+  [Constant]
>   readonly attribute SVGStringList systemLanguage;
[...]
>diff --git a/dom/webidl/SVGViewElement.webidl b/dom/webidl/SVGViewElement.webidl
>--- a/dom/webidl/SVGViewElement.webidl
>+++ b/dom/webidl/SVGViewElement.webidl
> interface SVGViewElement : SVGElement {
>+  [Constant]
>   readonly attribute SVGStringList viewTarget;
> };

I think (?) these are the only non-SVGAnimated*** types in this patch, which makes them stand apart from the rest of this patch.  Not sure if that merits splitting them off into a separate patch; just pointing it out since it seems slightly at odds with comment 1.
Attached patch preserve.txtSplinter Review
Attachment #8554807 - Flags: review?(dholbert)
I'll remove the non*SVG-Animated stuff into another patch.
If we had a loop and we invoke a JIT then it can omit repeated calls (caching the result).
Attached patch const.txtSplinter Review
Attachment #8554298 - Attachment is obsolete: true
Attachment #8554298 - Flags: review?(dholbert)
Attachment #8554815 - Flags: review?(dholbert)
(In reply to Robert Longson from comment #10)
> If we had a loop and we invoke a JIT then it can omit repeated calls
> (caching the result).

Ah, OK.

So if we accidentally don't quite hold up our end of the "[Constant]" bargain (and, say, return different values on subsequent calls), then the worst thing this bug could cause would be to give websites slightly different behavior in the JIT case (when repeated calls are optimized away) vs. the non-JIT case (where we'd expose a pre-existing buggy returning-different-values issue)?

If so, it sounds like we should go ahead & mark all of these things [Constant], because any discrepancies are already bugs that could cause this sort of issue in web content. Perhaps worth auditing these functions at some point, but (if I'm understanding correctly) I shouldn't lose sleep over r+'ing this without doing that audit myself. :)

Does this sound sane?
Actually I don't think bug 839711 is related, that's about handing out new baseVal and animVal sub-objects, not about the value itself being different and SVGAnimatedPreserveAspectRatio::ToDOMAnimatedPreserveAspectRatio itself does hand out the same object every time.
(In reply to Daniel Holbert [:dholbert] from comment #12)
> 
> Ah, OK.
> 
> So if we accidentally don't quite hold up our end of the "[Constant]"
> bargain (and, say, return different values on subsequent calls), then the
> worst thing this bug could cause would be to give websites slightly
> different behavior in the JIT case (when repeated calls are optimized away)
> vs. the non-JIT case (where we'd expose a pre-existing buggy
> returning-different-values issue)?

That's my understanding.

> 
> If so, it sounds like we should go ahead & mark all of these things
> [Constant], because any discrepancies are already bugs that could cause this
> sort of issue in web content. Perhaps worth auditing these functions at some
> point, but (if I'm understanding correctly) I shouldn't lose sleep over
> r+'ing this without doing that audit myself. :)

Note that many files already have *Animated stuff marked as [Constant] e.g. http://mxr.mozilla.org/mozilla-central/source/dom/webidl/SVGPatternElement.webidl#14

And if you want a preserveAspectRatio example: http://mxr.mozilla.org/mozilla-central/source/dom/webidl/SVGImageElement.webidl

> 
> Does this sound sane?

I think so.
Comment on attachment 8554727 [details] [diff] [review]
all that's left of the pure changes

r=me
Attachment #8554727 - Flags: review?(bzbarsky) → review+
(In reply to Robert Longson from comment #13)
> Actually I don't think bug 839711 is related, that's about handing out new
> baseVal and animVal sub-objects, not about the value itself being different
> and SVGAnimatedPreserveAspectRatio::ToDOMAnimatedPreserveAspectRatio itself
> does hand out the same object every time.

Nice, OK. (I would've noticed that too if I'd read beyond the bug summary. :))
Comment on attachment 8554807 [details] [diff] [review]
preserve.txt

This is great! Needs a commit message obviously, but I'll trust you to provide a sane one. :)
Attachment #8554807 - Flags: review?(dholbert) → review+
Comment on attachment 8554815 [details] [diff] [review]
const.txt

(I asked for a sanity-check on comment 12 in IRC, and it sounds like we're good to optimistically annotate these as [Constant] -- at least, mccr8 basically restated comment 10, and said he couldn't think of a way that the annotation could cause a security issue.)

so, r=me on this, given that it matches our intent, as stated in e.g. bug 839711 comment 2.
Attachment #8554815 - Flags: review?(dholbert) → review+
Comment on attachment 8554815 [details] [diff] [review]
const.txt

Still need a dom peer review to check this in.
Attachment #8554815 - Flags: superreview?(bzbarsky)
It bothers me to knowingly lie about [Constant] in that it causes weird issues where the behavior of a site depends on how many times the function has executed...  I _think_ there aren't memory safety issues or anything (or at least not any more so than a site could create via manual loop-hoisting and whatnot).  But the inconsistent behavior is not great.

Do we have a concrete plan for fixing up the non-constantness?
Also, in the cases when we do return a different thing, are we at least 100% guaranteed that it's the same type of thing as before?
(In reply to Boris Zbarsky [:bz] from comment #20)
> It bothers me to knowingly lie about [Constant]

To be clear, I don't think we'd be knowingly lying about it. (Though maybe I missed something.)

My takeaway was: These all really *should* be able to be labeled as "[Constant]" -- and any methods that turn out to not hold up their part of that bargain are bugs which need filing & fixing.  Ideally we'd audit each one explicitly before adding this annotation, but I'm not sure that should be a requirement for landing this, given that the "return same object, to the extent that script can tell" is already something that we aim to do & and consider it a bug when we don't do it.

I agree that if there are any that we *know* aren't actually [Constant]-able right now, though, we shouldn't label them.  If you think we should take this a step further and actively audit all of these, I'm OK with that too, and I'll trust longsonr to do that if he's up for it. :)
(In reply to Boris Zbarsky [:bz] from comment #21)
> Also, in the cases when we do return a different thing, are we at least 100%
> guaranteed that it's the same type of thing as before?

(Again, I'm not aware of any such cases. But I'd expect that it would be, yes -- the same type of tearoff object.)
Comment on attachment 8554815 [details] [diff] [review]
const.txt

OK.  I think I'm OK with this as long as the types are guaranteed to match...
Attachment #8554815 - Flags: superreview?(bzbarsky) → superreview+
Attachment #8554807 - Flags: checkin+
Keywords: leave-open
Flags: in-testsuite-
Attachment #8554727 - Flags: checkin+
Attachment #8554815 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.