948488 - Hunt down all the places where DOM APIs return array COWs

Status: RESOLVED WORKSFORME

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

Description

[Boris Zbarsky [:bzbarsky]]

With bug 697343 these no longer have a working slice() method.  So we should make sure we don't return them.

Bobby, can you do a try push of a patch that would cause such cases to fail?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Need to track for web compat impact.

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ab2911997e19

Comment 3

[Bobby Holley (:bholley)]

So, looks like we run into quite a thicket of failures.

The two obvious ones that we need to clean up are CSP and browser element. I could add some code into AccessCheck to allow people explicitly create __exposedProps__ for indexed properties, and then fix up the various APIs to use that. But I think it's probably not worth actually shipping that mildly-tightened restriction, because it's not the end-state that we want. If we ship any changes to the policy here, I'd like it to be a straight-up "disallow exposing chrome arrays", and force people to create content arrays. But to do _that_ we probably want to first support Xrays to arrays, which is part of bug 914970. So...

I also don't really want to sink a bunch of time into getting our test suite green with a restriction we won't actually ship, because then it'll just regress again.

How do you want to proceed, Boris?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Why is it not worth shipping the mildly-tightened restriction?  I'd obviously prefer the full restriction, but the mild one should at least make it impossible to _accidentally_ return a COW like in the contacts case...

Also, are we really sure we need Xrays to arrays before we do the full restriction?  If we do, that's said, but if we don't, let's just do the full restriction.  ;)

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Why is it not worth shipping the mildly-tightened restriction?  I'd
> obviously prefer the full restriction, but the mild one should at least make
> it impossible to _accidentally_ return a COW like in the contacts case...

Because it potentially impacts addon authors, and the thing they're most likely to do to fix it (adding __exposedProps__) will stop working soon anyhow.
 
> Also, are we really sure we need Xrays to arrays before we do the full
> restriction?

Well, for example: if chrome owns the array, our fix for bug 853709 works. If content owns it, we need Xrays in order to avoid getting confused.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Because it potentially impacts addon authors

Hrm, yes.

So maybe the right solution is to make slice() work right on array COWs for now, then?

> If content owns it,

Then we have a problem, if we ever look at it, yes.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> > Because it potentially impacts addon authors
> 
> Hrm, yes.
> 
> So maybe the right solution is to make slice() work right on array COWs for
> now, then?

Or potentially, just not worry about it. COWs have always had quirks, and I don't know if any of the APIs we implement with them are widely-used enough outside of gaia to be at a real risk of breaking with something like that. Then again, maybe that's the wrong attitude. *shrug*

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, that's the thing.  I'm not sure what APIs are involved here, even, so I have no way to judge whether they're limited to gaia.....

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Well, that's the thing.  I'm not sure what APIs are involved here, even, so
> I have no way to judge whether they're limited to gaia.....

Well, this is a start: http://mxr.mozilla.org/mozilla-central/search?string=__exposedProps__

It obviously doesn't tell us about APIs that return _only_ primitives and arrays to content, but it's a pretty easy way to get 90% of the information we need.

The only way to tell for sure is to fix all the failures in comment 2 until they're green. But given the other things we need to do in this department, that does not feel like the best use of time for anyone qualified to do it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Well, this is a start:

But the insiduous thing with array COWs is that you can return them accidentally without __exposedProps__ and they will in fact work just fine... except for slice().

So we need to either make that stop happening or fix slice() on COWs or something.  Which one of those is the best use of time is an interesting question, sure

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> But the insiduous thing with array COWs is that you can return them
> accidentally without __exposedProps__ and they will in fact work just
> fine... except for slice().

That's not really true. There are a number of ways in which COWs already differ (subtly) from other objects. They've never really been 100%. Note that things like cowArray.forEach only started working a bit over a year ago.

> So we need to either make that stop happening or fix slice() on COWs or
> something.

In a vacuum, maybe. But for this to be a problem, all of the following would have to be  true:
* There is an API that accidentally returns Array COWs but does not do anything else that would require it to use __exposedProps__ (which would cause it to show up on our MXR list).
* This API is exposed to the web.
* This API is used widely enough that changing behavior with respect to Array.slice is a problem.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> There are a number of ways in which COWs already differ (subtly) from other objects.

OK, fair.

Again, I'm worried about "this web page used to work, and now it doesn't" scenarios.  I would _really_ like to avoid those.

Agreed on your three criteria, but it's hard to judge the latter two without having a handle on the list of APIs for that first.  Which is what I was hoping to gather in this bug so we can make an informed decision about the others.

Comment 13

[Benjamin Kerensa [:bkerensa]]

I agree we should track this because of the potential for web compat issues.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Note that in bug 949197 we're fixing slice() to work on COWs.  So I don't think there will be any web compat issues here that weren't already present in 28...

Comment 15

[Bobby Holley (:bholley)]

Given bug 1065186, marking this WFM.