1155342 - Disallow flagging [NewObject] methods with [Pure] or [Constant] or the corresponding [DependsOn] values

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Clearly something that's [NewObject] is not idempotent, so we shouldn't allow marking it [Pure].

It _could_ be marked as [DependsOn=DeviceState, Affects=Nothing], though, if it's side-effect-free. But [DependsOn=Nothing] and [DependsOn=DOMState] would be obvious lies.

We should enforce this in IDLInterfaceMember.validate.

Comment 1

[Jinank Jain[:jj]]

Hi I would like to work on this bug could you help me out how to start off with this ?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Have you already compiled Firefox?  If not, start with that.  If you have, then you want to be looking in dom/bindings/parser/WebIDL.py in the "validate" method of the IDLInterfaceMember class.

There are existing checks there for the self.dependsOn value.  You'd want to add another one: that if self.getExtendedAttribute("NewObject") then self.dependsOn is not "Nothing" and is not "DOMState".

Also probably a good idea to assert that self.dependsOn is in DependsOnValues.

Let me know if that's not enough to go on?

Comment 3

[Jinank Jain[:jj]]

if self.getExtendedAttribute("NewObject"):
   if self.dependsOn != "Nothing" and self.dependsOn != "DOMState":
      raise WebIDLError("self.dependsOn is in DependsOnValues", [self.location])

I got and error while building it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

You got the check backwards, no?  You're raising an error precisely in the case when you shouldn't.

Comment 5

[Jinank Jain[:jj]]

Then what should I do precisely ?

Comment 6

[Boris Zbarsky [:bzbarsky]]

You should raise an error if self.getExtendedAttribute("NewObject") and the dependsOn value is "Nothing" or "DOMState".

Comment 7

[Jinank Jain[:jj]]

Attachment: p1v1.diff

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, so now that's the right conditions.  The assertion about the self.dependsOn value being in DependsOnValues is missing, and the error message text is wrong.  It should be more like "A [NewObject] method is not idempotent, so it has to depend on something other than DOM state."

Comment 9

[Jinank Jain[:jj]]

Attachment: p1v2.diff

Comment 10

[Jinank Jain[:jj]]

Should I now go for a try push ?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8613084 [details] [diff] [review]
p1v2.diff

r=me.  Try push is good, yes.  Can you do that yourself, or should I do it?  Doing a build-only push on all platforms is enough here.

Comment 12

[Jinank Jain[:jj]]

Ya I could do it on my own but could you tell me what would be try message syntax

Comment 13

[Jinank Jain[:jj]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91f595cde124

Comment 14

[Jinank Jain[:jj]]

Any Updates ?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Did your try run pass?  If it did, you should add the "checkin-needed" keyword to this bug.

Comment 16

[Jinank Jain[:jj]]

There are some problem with windows could you please check those things I have posted the the treehereder link

Comment 17

[Boris Zbarsky [:bzbarsky]]

Looks like your push was on top of a changeset that doesn't actually build on Windows.  I recommend picking a known-working changeset to do a try push on top of....

Comment 18

[Jinank Jain[:jj]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffa28afb8c61

Comment 19

[Jinank Jain[:jj]]

Still there are some bug with android build could you help me with that :)

Comment 20

[Boris Zbarsky [:bzbarsky]]

That looks like infrastructure issues that have nothing to do with your patch.  This should be good to check in, but you need an actual commit message and user information in that diff, right?

Comment 21

[Jinank Jain[:jj]]

Attachment: p1v3.diff

Comment 22

[Boris Zbarsky [:bzbarsky]]

Still needs a commit message.  The commit message will typically look something like:

  Bug NNNNN.  The stuff I changed.  r=reviewer

so in this case

  Bug 1155342.  Disallow flagging a [NewObject] method with a [DependsOn] value that implies it might return the same value when called twice.  r=bzbarsky

or so.

Comment 23

[Jinank Jain[:jj]]

Attachment: p1v3.diff

Comment 24

[Boris Zbarsky [:bzbarsky]]

But without the try syntax in the commit message, yes?  ;)

Comment 25

[Jinank Jain[:jj]]

Attachment: p1v3.diff

Comment 26

[Boris Zbarsky [:bzbarsky]]

Much better, yes.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6477aa2270

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7f6477aa2270

Comment 29

[Boris Zbarsky [:bzbarsky]]

Jinank Jain, thank you for fixing this!