Closed Bug 871510 Opened 11 years ago Closed 11 years ago

Some event handlers appear as undefined, hiding the appropriate null values in the prototype

Categories

(DevTools :: Console, defect, P2)

Product:
DevTools
Component:
Console
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 24

People

(Reporter: past, Assigned: past)

References

Details

Attachments

(1 file)

Patch v1
5.13 KB, patch
msucan
: review+
Details | Diff | Splinter Review 
STR:
1. Visit http://htmlpad.org./debugger/ and open the debugger
2. Click the 'Click me' button and inspect the pAsProto variable
3. The variables view shows onmouseenter and onmouseleave properties on the instance, but with undefined values.

This seems wrong, since such event listeners on DOM elements have the value null when not set, which is indeed the case for pAsProto.__proto__.  The same thing happens for the docAsProto variable as well.

This started after bug 870220 landed.
We discussed this with Mihai and our theory is that this happens because the new code tries to apply native getters on the first object in the prototype chain, instead of in the first object in the prototype chain going backwards that doesn't throw, like it used to.

Boris, we'd like a clarification on how WebIDL attributes are supposed to work in the following (corner) case, to make sure we're on the right track. Say we have an object that is created with a DOM element as its prototype:

var p = document.querySelector("p");
var pAsProto = Object.create(p);

Is a native getter for a WebIDL attribute on the prototype chain of p supposed to be called on p or on pAsProto?

Using the STR form comment 0 shows the current behavior in both the debugger and console.
> Is a native getter for a WebIDL attribute on the prototype chain of p supposed to be
> called on p or on pAsProto?

On p.  So for example, if you do:

  var p = document.querySelector("p");
  var pAsProto = Object.create(p);
  pAsProto.firstChild; // Throws exception

then you get an exception.
OK, thanks. One other thing I meant to ask is whether there is something special about onmouseenter and onmouseleave in the case of p and document, and onreadystatechange in the case of document. These appear to not throw when called with the wrong object as |this|, which pointed to the bug in the first place.
Yes, there is.  Those particular properties are commonly called with bogus this objects on the web, so browsers (and now the spec) implemented a workaround: when called with an incorrect this object, these properties simply do nothing.  See the LenientThis annotation in the IDL for these properties.
Thanks, it all makes perfect sense now.
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch Patch v1 — — Splinter Review 
I tried various complicated solutions, but in the end the simplest approach was the best. The WebIDL spec says that applying native getters on the wrong object for attributes with the LenientThis extended attribute returns undefined, so just handling that suffices. I added a test that fails without the change.
Attachment #749948 - Flags: review?(mihai.sucan)
Comment on attachment 749948 [details] [diff] [review]
Patch v1

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

Works as advertised. Thanks for the patch!

::: toolkit/devtools/webconsole/test/test_object_actor_native_getters_lenient_this.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html lang="en">
> +<head>
> +  <meta charset="utf8">
> +  <title>Test for the native getters in object actors</title>

Can you please update the test title to tell what it checks?

@@ +14,5 @@
> +<script class="testbody" type="text/javascript;version=1.8">
> +SimpleTest.waitForExplicitFinish();
> +
> +let expectedProps = [];
> +let expectedSafeGetters = [];

Are these two needed?

@@ +72,5 @@
> +
> +  info("check ownProperties");
> +  checkObject(props, expectedProps);
> +  info("check safeGetterValues");
> +  checkObject(aResponse.safeGetterValues, expectedSafeGetters);

Are these two calls to checkObject() needed? You already check if keys.length == 0.
Attachment #749948 - Flags: review?(mihai.sucan) → review+
Updated per review comments and landed:
https://hg.mozilla.org/integration/fx-team/rev/b865c805fd59
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b865c805fd59
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: