699826 - Nodelists should expose proto properties when indexing outside the list range

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

At least if I read the spec right.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix nodelist access out of bounds to correctly forward to the expando object and proto.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(I believe the spec actually doesn't allow setting numeric expandos.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  Well, we allow setting them at the moment; we just didn't allow getting them after that.  That's detectable from script, I believe (e.g. per spec if I read it right the expando set should throw in strict mode).

I could change this patch to just forward to proto and file a separate bug on disallowing setting numeric expandos, I guess.  Peter?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Hmm.  Well, we allow setting them at the moment; we just didn't allow
> getting them after that.  That's detectable from script, I believe (e.g. per
> spec if I read it right the expando set should throw in strict mode).

Yep. I hijacked bug 371711 for that.

Comment 5

[Cameron McCormack (:heycam)]

Yeah, comment 2 is right; it's not possible to set numeric expandos, so it's just the prototype that you need to look up.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix nodelist access out of bounds to correctly forward to the proto.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 572579 [details] [diff] [review]
Fix nodelist access out of bounds to correctly forward to the proto.

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

::: js/xpconnect/src/dombindings.cpp
@@ +1030,4 @@
>          }
>      }
>  
> +    if (getFromExpandoObject) {

Could you add an assert here for !xpc::WrapperFactory::IsXrayWrapper(proxy)? We should really do that wherever we use the expando without checking because we rely on the way Xray wrappers implement their proxy handler.

@@ +1079,5 @@
>      if (!JS_IndexToId(cx, index, &id))
>          return false;
>  
> +    // if hasIndexGetter, we skip the expando object
> +    if (hasIndexGetter) {

I think this should be |if (!hasIndexGetter)|.
Same thing about asserting !xpc::WrapperFactory::IsXrayWrapper(proxy).

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Could you add an assert here for !xpc::WrapperFactory::IsXrayWrapper(proxy)?

I guess I should assert that at the top of both get() and getElementIfPresent(), right?  Why is this OK to assert?

> I think this should be |if (!hasIndexGetter)|.

Yes, good catch!

Comment 9

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9543018be98c

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #8)
> I guess I should assert that at the top of both get() and
> getElementIfPresent(), right?

Yes.

> Why is this OK to assert?

Because Xray wrappers call ProxyHandler::get which calls getPropertyDescriptor, and the DOM bindings getPropertyDescriptor checks for IsXrayWrapper before returning expandos.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 572579 [details] [diff] [review]
Fix nodelist access out of bounds to correctly forward to the proto.

We should probably take this on aurora: this is fixing a potential web compat regression from the new nodelist bindings.

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 572579 [details] [diff] [review]
Fix nodelist access out of bounds to correctly forward to the proto.

[Triage Comment]
Please re-request for aurora (or rather beta for FF9 if necessary) once this lands on m-c.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/9543018be98c

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 572579 [details] [diff] [review]
Fix nodelist access out of bounds to correctly forward to the proto.

Per comment 12, requesting approval.

Comment 15

[LegNeato]

[triage comment]
bz, does this also affect 9? 8? Is it new to 10 (on aurora)? How would the web compat hit show up?

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is new in 10, since it's a regression from bug 648801.

The web compat hit would show up via web pages getting undefined when they expected to get some other value.  I don't thnk it would be _common_ by any means, but it would be possible.

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 572579 [details] [diff] [review]
Fix nodelist access out of bounds to correctly forward to the proto.

[Triage Comment]
* Given that we're in our second week of the cycle, and this is a regression in 10, let's take this for aurora.

Comment 18

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6f4131d4bd72

Comment 19

[u279076]

Is this something QA can verify?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yes.  Just write a testcase that sets numeric properties on the prorotype and see whether they show up on the list.

Comment 21

[u279076]

I don't know how to write said testcase. I'll leave this as [qa?] to see if someone else can come up with a testcase or at least help me to write one.

Comment 22

[Boris Zbarsky [:bzbarsky]]

There's a testcase in the patch, but that passes, of course, since it runs as part of our automated tests...

Comment 23

[u279076]

(In reply to Boris Zbarsky (:bz) from comment #22)
> There's a testcase in the patch, but that passes, of course, since it runs
> as part of our automated tests...

It sounds like this is covered "enough" then and can be safe marked VERIFIED...no?

Comment 24

[Boris Zbarsky [:bzbarsky]]

I won't claim that I understand our bug verification criteria or motivations, so I have no idea...

Comment 25

[u279076]

I'm going to mark this as [qa-] for now as the fix is "verified" by automated tests and QA has not done anything to verify the fix directly (nor will we in the near term). Should anyone decide that this needs verification by QA in the future, please set the whiteboard tag to [qa+].

Thanks