1373042 - ExportHelpers has a nice long comment about FileList that doesn't match reality

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

The comment at talks about how we can rely on FileList being implemented as an XPCWN and so on, but they haven't been XPCWN in a while.  Sadly, UnwrapReflectorToISupports() still works on Web IDL objects, so this codepath is still returning true for FileList.

Does that mean we're doing unsafe things when mOptions->wrapReflectors is false?  Should we just remove this FileList special-case?  Something else?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Er, the comment at http://searchfox.org/mozilla-central/rev/db5e3a34d4b26f6c8e97fde4a16bf41f10f9b146/js/xpconnect/src/ExportHelpers.cpp#158-161

Comment 2

[Bobby Holley (:bholley)]

Nice catch - I think this is ok though. The impact of the bug is that we'll effectively always have the wrapReflectors=true behavior for FileList, which potentially means that somebody cloning an object graph containing a FileList might get have the error appear lazily (via a security wrapper denial when content tries to access the object) rather than eagerly (when the clone happens).

In other words, security wrappers save us. So the safest thing is just to leave this be for the next two cycles and then remove the special case once XPCOM addons go away.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Do we have a tracking bug for things we can do post-57?

Comment 4

[Andrew McCreight [:mccr8]]

Bug 1347507?

Comment 5

[Andrew Overholt [:overholt]]

I searched for at least 15 minutes and couldn't find bug 1347507. Thanks, Andrew!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1373042. Remove the FileList special-case in ExportHelpers.

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/autoland/rev/8e197e2c37390f81ced8c150c7781fe7f0d05fb1

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8e197e2c3739