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

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: gera2ld, Assigned: bzbarsky)

Tracking

57 Branch
mozilla64
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox64 fixed)

Details

Attachments

(3 attachments)

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.
Component: Untriaged → WebExtensions: General
Product: Core → Toolkit
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1208775
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...
Flags: needinfo?(tomica)
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.
Flags: needinfo?(tomica)
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...
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.
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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
(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
Product: Toolkit → WebExtensions
Component: General → XPConnect
Product: WebExtensions → Core
Kris, is there a plan here?
Flags: needinfo?(kmaglione+bmo)
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.
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.
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?
Flags: needinfo?(jdemooij)
One option is to give JS_NewEnumerateStandardClasses an optional boolean arg for "include resolved ones".
I guess that doesn't work because JS_NewEnumerateStandardClasses is used as a class newenumerate hook.  So we would really need a separate method.
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...
Flags: needinfo?(jdemooij)
Assignee: nobody → bzbarsky
Status: REOPENED → ASSIGNED
Flags: needinfo?(kmaglione+bmo)
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).
Attachment #9012665 - Flags: review?(bobbyholley) → review+
> I think the commit message should be something like "enumerate" rather than "expose"

Done.
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.
Attachment #9012663 - Flags: review?(jdemooij) → review+
Would it make sense to also move the resolving code for builtins from XrayTraits::resolveOwnProperty to DOMXrayTraits::resolveOwnProperty, for symmetry?
Priority: -- → P2
> 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...
Pushed by bzbarsky@mozilla.com:
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
Need to adjust the test expectations.  The test actually has comments about how it's working around this bug for NaN and Infinity!
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...
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Attachment #9012959 - Flags: review?(bobbyholley) → review+
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2396739f4dfc
https://hg.mozilla.org/mozilla-central/rev/747286879049
Status: ASSIGNED → RESOLVED
Closed: 2 years ago11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.