Closed Bug 883358 Opened 11 years ago Closed 11 years ago

JS_DefineProperties should set a useful name on the getters and and setters

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

Would be nice for bindings.
With the upcoming patches, this testcase: 

  <script>
    Object.getOwnPropertyDescriptor(Document.prototype, "documentElement").get.call({})
  </script>
  <script>
    Object.getOwnPropertyDescriptor(Element.prototype, "innerHTML").set.call({})
  </script>
  <script>
    Document.prototype.querySelector.call({});
  </script>

leads to this in the console:

  [17:07:36.513] TypeError: 'documentElement' getter called on an object that does not implement interface Document. @ file:///private/tmp/test.html:4
  [17:07:36.515] TypeError: 'innerHTML' setter called on an object that does not implement interface Element. @ file:///private/tmp/test.html:7
  [17:07:36.516] TypeError: 'querySelector' called on an object that does not implement interface Document. @ file:///private/tmp/test.html:10

which I consider a win.  ;)
Whiteboard: [need review]
By the way, there are differences of opinion as to what, if anything, should go around the {0} in those error messages.  Some like my single quotes, some want nothing to ease external tool processing, some want pipes and then teaching the console about them to do... something.
Attachment #762911 - Flags: review?(bugs) → review+
Comment on attachment 762907 [details] [diff] [review]
part 1.  Call setGuessedAtom on getters/setters set via JS_DefineProperties.

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

yep
Attachment #762907 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 762907 [details] [diff] [review]
part 1.  Call setGuessedAtom on getters/setters set via JS_DefineProperties.

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

I vote for switching to JS_NewFunctionById and actually giving these accessors appropriate names for real, not just setGuessedAtom. r=me with or without that tweak.
Attachment #762907 - Flags: review+
Attachment #762907 - Attachment is obsolete: true
Summary: JS_DefineProperties should setGuessedAtom on the getters and and setters → JS_DefineProperties should a useful name on the getters and and setters
Depends on: 882653
Comment on attachment 763702 [details] [diff] [review]
part 1.  When creating getter/setter functions for the JSPROP_NATIVE_ACCESSORS case in JS_DefineProperties/JS_DefineProperty, give them the name of the property.

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

I like it!
Attachment #763702 - Flags: review?(tschneidereit) → review+
   https://hg.mozilla.org/integration/mozilla-inbound/rev/757a3f2e5de6
   https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e6522257cb

With tests for this one and bug 882653.
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla24
>+  try {
>+    document.createElement("canvas").getContext("webgl");
>+    // If that didn't throw, our WebGL-using test should be safe
>+    tests.push(
>+        [ 'document.createElement("canvas").getContext("webgl").uniform1fv(null, new RegExp())',
>+	"Argument 2 is not valid for any of the 2-argument overloads of WebGLRenderingContext.uniform1fv.",
>+	"regexp passed for a sequence" ]
>+    );
>+  } catch (e) {
>+    // No WebGL in some cases, apparently
>+    todo(false, "No WebGL here?");
>+  }

We don't support getContext("webgl") for mobile yet. See bug 870232.
It should be something like:

  ['webgl', 'experimental-webgl'].forEach(function(c){var canvas = document.createElement("canvas"); var gl = canvas.getContext(c); if (!gl) return; gl.uniform1fv(null, new RegExp())})

And this should never fail to get a WebGL context.
>+    document.createElement("canvas").getContext("webgl");
>+    // If that didn't throw, our WebGL-using test should be safe

This should not throw but should return null. If it throws, it is a bug in itself.
Unfortunately (maybe expected given the last couple comments), the follow-up push didn't fix (or workaround) the Android mochitest-3 failures. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/04b177645a6b
> We don't support getContext("webgl") for mobile yet

Ah, that explains everything.  One web indeeed.  :(

> This should not throw but should return null. If it throws, it is a bug in itself.

We have various places in our test suite that seem to think it throws on Mac when there's no WebGL support, fwiw.
Summary: JS_DefineProperties should a useful name on the getters and and setters → JS_DefineProperties should set a useful name on the getters and and setters
I tried using "experimental-webgl" but then b2g throws with some random "Failure" exception that I can't observe on Android.

So I gave up on WebGL and found something else to exercise overload resolution.  Pushed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/1cec8b5a9ac2
   https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc3d2853ee3
You need to log in before you can comment on or make changes to this bug.