Last Comment Bug 699826 - Nodelists should expose proto properties when indexing outside the list range
: Nodelists should expose proto properties when indexing outside the list range
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla11
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 648801
  Show dependency treegraph
 
Reported: 2011-11-04 09:27 PDT by Boris Zbarsky [:bz]
Modified: 2015-10-16 11:44 PDT (History)
7 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Fix nodelist access out of bounds to correctly forward to the expando object and proto. (6.67 KB, patch)
2011-11-04 11:50 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Fix nodelist access out of bounds to correctly forward to the proto. (5.70 KB, patch)
2011-11-07 12:34 PST, Boris Zbarsky [:bz]
peterv: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2011-11-04 09:27:40 PDT
At least if I read the spec right.
Comment 1 Boris Zbarsky [:bz] 2011-11-04 11:50:35 PDT
Created attachment 572034 [details] [diff] [review]
Fix nodelist access out of bounds to correctly forward to the expando object and proto.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-11-04 12:15:11 PDT
(I believe the spec actually doesn't allow setting numeric expandos.)
Comment 3 Boris Zbarsky [:bz] 2011-11-04 12:33:10 PDT
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 (⌚ UTC+1/+2) 2011-11-04 13:58:05 PDT
(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) 2011-11-05 11:28:16 PDT
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 [:bz] 2011-11-07 12:34:44 PST
Created attachment 572579 [details] [diff] [review]
Fix nodelist access out of bounds to correctly forward to the proto.
Comment 7 Peter Van der Beken [:peterv] 2011-11-08 03:21:33 PST
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 [:bz] 2011-11-08 04:58:14 PST
> 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 10 Peter Van der Beken [:peterv] 2011-11-08 05:04:16 PST
(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 [:bz] 2011-11-08 09:42:30 PST
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] 2011-11-08 14:52:07 PST
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] 2011-11-09 05:18:12 PST
https://hg.mozilla.org/mozilla-central/rev/9543018be98c
Comment 14 Boris Zbarsky [:bz] 2011-11-09 11:08:24 PST
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 christian 2011-11-15 14:45:32 PST
[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 [:bz] 2011-11-15 14:49:35 PST
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] 2011-11-17 14:38:23 PST
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 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 14:23:02 PST
Is this something QA can verify?
Comment 20 Boris Zbarsky [:bz] 2011-12-28 14:24:49 PST
Yes.  Just write a testcase that sets numeric properties on the prorotype and see whether they show up on the list.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 16:16:28 PST
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 [:bz] 2011-12-28 16:23:16 PST
There's a testcase in the patch, but that passes, of course, since it runs as part of our automated tests...
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 16:24:46 PST
(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 [:bz] 2011-12-28 16:32:12 PST
I won't claim that I understand our bug verification criteria or motivations, so I have no idea...
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-29 10:02:35 PST
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

Note You need to log in before you can comment on or make changes to this bug.