Mark methods and attributes pure and const where appropriate

RESOLVED FIXED in mozilla38

Status

()

Core
SVG
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

unspecified
mozilla38
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 8554298 [details] [diff] [review]
const.txt

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)
(Assignee)

Comment 2

4 years ago
Created attachment 8554299 [details] [diff] [review]
Stuff I think is [pure]
Assignee: nobody → longsonr
Attachment #8554299 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
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-
(Assignee)

Updated

4 years ago
Attachment #8554298 - Flags: review?(dholbert)
(Assignee)

Comment 5

4 years ago
Created attachment 8554727 [details] [diff] [review]
all that's left of the pure changes
Attachment #8554299 - Attachment is obsolete: true
Attachment #8554727 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #8554727 - Attachment is patch: true
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 8

4 years ago
Created attachment 8554807 [details] [diff] [review]
preserve.txt
Attachment #8554807 - Flags: review?(dholbert)
(Assignee)

Comment 9

4 years ago
I'll remove the non*SVG-Animated stuff into another patch.
(Assignee)

Comment 10

4 years ago
If we had a loop and we invoke a JIT then it can omit repeated calls (caching the result).
(Assignee)

Comment 11

4 years ago
Created attachment 8554815 [details] [diff] [review]
const.txt
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?
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Comment 14

4 years ago
(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+
(Assignee)

Comment 19

4 years ago
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+
(Assignee)

Updated

4 years ago
Attachment #8554807 - Flags: checkin+
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Updated

4 years ago
Flags: in-testsuite-
(Assignee)

Updated

4 years ago
Attachment #8554727 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8554815 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/c579974cfa31
https://hg.mozilla.org/mozilla-central/rev/bf6dcc8a0d57
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.