1396482 - Object.getOwnPropertyNames(window) is missing some native functions in WebExtension content page

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[gera2ld]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170903220032

Steps to reproduce:

1. In normal web page, open console, type `console.log(Object.getOwnPropertyNames(window).includes('Array'))`, got `true`.
2. In WebExtension content page, `console.log(Object.getOwnPropertyNames(window).includes('Array'))`, got `false`.


Actual results:

See above.


Expected results:

'Array', 'String', etc should be included in `Object.getOwnPropertyNames(window)` either in web page or WebExtension content page, but actually missing in WebExtension content page. This has a bad effect on my WebExtension ported from Chrome since it worked well on Chrome.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Are you sure this is a duplicat of bug 1208775?  I can think of at least two approaches that fix bug 1208775 but don't affect this issue...

Comment 3

[Tomislav Jovanovic :zombie]

I could be wrong, bug looking at other dupes of bug 1208775, it is not (only) specifically about `this === window` being false, but about all the consequences of `window` not being the global in content scripts (which would also include this).

If you are working on a solution for bug 1208775 that doesn't also cover this issue though, feel free to reopen/undupe/depend as you see appropriate.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm not working on a solution.  But ok, it want's clear to me that the plan was to fix "window to be the global" as opposed to fixing "this" to be the targeted window...

Comment 5

[Boris Zbarsky [:bzbarsky]]

I guess in either case, my approach would be to mark dependent, not dup.  Otherwise the chance that anyone tests the behavior here once bug 1208775 is fixed is pretty slim.  But not my component.

Comment 6

[Kris Maglione [:kmag]]

It looks like this is more about X-ray wrappers around `window` than they are about top-level `this` not pointing to the window wrapper.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6)
> It looks like this is more about X-ray wrappers around `window` than they
> are about top-level `this` not pointing to the window wrapper.

*than it is about

Comment 8

[Andrew Overholt [:overholt]]

Kris, is there a plan here?

Comment 9

[Peter Van der Beken [:peterv]]

We do have code in XrayWrapper for resolving standard classes on global objects. It's not clear to me that the target here is actually the window. If it is then there might be a bug in that code, but if it isn't then the code won't kick in.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We have code for making "window.Array" work, in XrayTraits::resolveOwnProperty.

I don't think we have anything in DOMXrayTraits::enumerateNames for standard classes.

We might be able to just throw a JS_NewEnumerateStandardClasses in there in the global case.  I'll give that a shot.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So one issue is that JS_NewEnumerateStandardClasses only returns the names of _unresolved_ standard classes.  Here we really want to get the names of all standard classes, because we're not doing an enumerate on the object itself, so whether they've been resolved there or not does not matter.

Jan, would adding an API for that be OK?  Any preferences on naming it?

Comment 12

[Boris Zbarsky [:bzbarsky]]

One option is to give JS_NewEnumerateStandardClasses an optional boolean arg for "include resolved ones".

Comment 13

[Boris Zbarsky [:bzbarsky]]

I guess that doesn't work because JS_NewEnumerateStandardClasses is used as a class newenumerate hook.  So we would really need a separate method.

Comment 14

[Jan de Mooij [:jandem]]

Adding an API for this is fine I think. Internally we could use a helper function with a bool argument to avoid duplication.

Maybe JS_NewEnumerateStandardClassesIncludingResolved? Pretty long but clear...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add an API for enumerating all the standard class names on a global

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Expose JS standard classes on Window Xrays, just like we expose WebIDL interfaces

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 9012665 [details] [diff] [review]
part 2.  Expose JS standard classes on Window Xrays, just like we expose WebIDL interfaces

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

I think the commit message should be something like "enumerate" rather than "expose", since they're already exposed (just not listed when querying names).

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I think the commit message should be something like "enumerate" rather than "expose"

Done.

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 9012663 [details] [diff] [review]
part 1.  Add an API for enumerating all the standard class names on a global

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

::: js/src/jsapi.cpp
@@ +1136,3 @@
>  {
>      if (enumerableOnly) {
>          // There are no enumerable lazy properties.

I was wondering if this is correct for the includeResolved case. Maybe change the comment to "There are no enumerable standard classes."?

::: js/src/jsapi.h
@@ +957,5 @@
>  extern JS_PUBLIC_API(bool)
>  JS_EnumerateStandardClasses(JSContext* cx, JS::HandleObject obj);
>  
> +/**
> + * Fill "properties" with a list of standard class names that have not yet been

Thanks for adding this comment.

Comment 20

[Peter Van der Beken [:peterv]]

Would it make sense to also move the resolving code for builtins from XrayTraits::resolveOwnProperty to DOMXrayTraits::resolveOwnProperty, for symmetry?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Maybe change the comment to "There are no enumerable standard classes."?

I will do that.

> Would it make sense to also move the resolving code for builtins from
> XrayTraits::resolveOwnProperty to DOMXrayTraits::resolveOwnProperty

I was a bit torn about that.  I guess we could do that instead of the maybe-move-enumeration-to-superclass comment I have...

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c23086c12393
part 1.  Add an API for enumerating all the standard class names on a global.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9b27320d6e
part 2.  Enumerate JS standard classes on Window Xrays, just like we enumerateWebIDL interfaces.  r=bholley

Comment 23

[Andreea Pavel [:apavel]]

Backed out for failing dom/tests/mochitest/general/test_interfaces.html 

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending,running,success,testfailed,busted,exception&classifiedState=unclassified&searchStr=m-e10s(9)&selectedJob=202238171&revision=8c9b27320d6e37b77631980d19607d5c58717896

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=202238171&repo=mozilla-inbound&lineNumber=23377

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/59496d5915844c01b3aa71894e35cfedf347443f

Comment 24

[Boris Zbarsky [:bzbarsky]]

Need to adjust the test expectations.  The test actually has comments about how it's working around this bug for NaN and Infinity!

Comment 25

[Boris Zbarsky [:bzbarsky]]

I guess there is one question there: with this change Object.getOwnPropertyNames will return a list including "Infinity" and "NaN" but Xrays don't produce property descriptors for those.  Bobby, are you ok with that?   If not, I can probably just add support for those two properties in Xrays too...

Comment 26

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #25)
> I guess there is one question there: with this change
> Object.getOwnPropertyNames will return a list including "Infinity" and "NaN"
> but Xrays don't produce property descriptors for those.  Bobby, are you ok
> with that?   If not, I can probably just add support for those two
> properties in Xrays too...

I don't have a strong opinion. In general I don't care, though it's plausible that it could cause some weird WebExtension bustage whereby somebody iterates and expects to be able to look up those same properties. I'm fine with either seeing what kmag thinks or just adding support.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the test and implement Xray support for Infinity and NaN

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2396739f4dfc
part 1.  Add an API for enumerating all the standard class names on a global.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/747286879049
part 2.  Enumerate JS standard classes on Window Xrays, just like we enumerateWebIDL interfaces.  r=bholley

Comment 29

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2396739f4dfc
https://hg.mozilla.org/mozilla-central/rev/747286879049