Closed Bug 948488 Opened 11 years ago Closed 10 years ago

Hunt down all the places where DOM APIs return array COWs

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox29 --- affected

People

(Reporter: bzbarsky, Unassigned)

References

Details

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?
Flags: needinfo?(bobbyholley+bmo)
Blocks: 697343
Need to track for web compat impact.
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?
Flags: needinfo?(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. ;)
Flags: needinfo?(bzbarsky)
(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.
> 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.
(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*
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.....
(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.
> 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
(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.
> 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.
I agree we should track this because of the potential for web compat issues.
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...
Given bug 1065186, marking this WFM.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.