Object.getOwnPropertyNames(window) is missing a bunch of things

RESOLVED FIXED in mozilla19

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

Trunk
mozilla19
x86_64
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fuzzblocker])

Attachments

(8 attachments, 1 obsolete attachment)

1.20 KB, text/html
Details
6.30 KB, patch
jst
: review+
Details | Diff | Splinter Review
5.98 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
8.55 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
7.69 KB, patch
Details | Diff | Splinter Review
948 bytes, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
6.18 KB, patch
past
: review+
Details | Diff | Splinter Review
1.33 KB, patch
khuey
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
(Split from bug 743171.)

In the browser,
  Object.getOwnPropertyNames(window)
is missing a bunch of items... until they are used.

A fix for this (or a solid workaround) would help my DOM fuzzer find objects to mess with and functions to call :)

Examples:

* http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#1283
** Image
** Audio

* http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfoClasses.h
** USSDReceivedEvent

* http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Bindings.conf
** PerformanceTiming

* http://mxr.mozilla.org/mozilla-central/source/dom/webidl/WebIDL.mk

Maybe this is a bug in http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptNameSpaceManager.cpp ?
Looks like the fundamental issue is that nsWindowSH::Enumerate only enumerates standard classes, and not all the things from the namespace manager, right?
Created attachment 676913 [details]
Testcase
Which may be the same as bug 576449?
So I guess the first question is whether it's OK to just resolve everything from the namespace manager when we enumerate or whether we should somehow try to use NewEnumerate... keeping track of where we are in the list would be a PITA.
Created attachment 676916 [details] [diff] [review]
It's ugly, but it works

Johnny, trying you because Peter is backlogged.  Note the comment about maybe changing Enumerate to avoid this when doing for...in.  Let me know if you want that here.
Attachment #676916 - Flags: review?(jst)
Assignee: nobody → bzbarsky
Whiteboard: [fuzzblocker] → [need review][fuzzblocker]
And I suppose I could keep track in a boolean somewhere that we did this and not do it again, if we really had to...
On my machine, with this patch getOwnPropertyNames on window takes about 15ms.  Without it takes closer to 0.3ms....  But note that we end up with 600-some names, not 71.

If we don't clear scopes anymore, I guess I could stick a boolean on nsGlobalWindow that says we did all this rigmarole, if we care.
Comment on attachment 676916 [details] [diff] [review]
It's ugly, but it works

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

::: dom/base/nsDOMClassInfo.cpp
@@ +5599,5 @@
> +      return NS_OK;
> +    }
> +
> +    // Now resolve everything from the namespace manager
> +    nsScriptNameSpaceManager *nameSpaceManager =

* to the left, please

::: dom/base/nsScriptNameSpaceManager.cpp
@@ +800,5 @@
> +  void* closure;
> +};
> +
> +static PLDHashOperator
> +EnumerateGlobalName(PLDHashTable*, PLDHashEntryHdr *hdr, uint32_t,

Same here

@@ +803,5 @@
> +static PLDHashOperator
> +EnumerateGlobalName(PLDHashTable*, PLDHashEntryHdr *hdr, uint32_t,
> +                    void* aClosure)
> +{
> +  GlobalNameMapEntry *entry = static_cast<GlobalNameMapEntry *>(hdr);

.

::: dom/base/test/test_window_enumeration.html
@@ +24,5 @@
> +var actualProps = Object.getOwnPropertyNames(window);
> +
> +for (var i = 0; i < expectedProps.length; ++i) {
> +  isnot(actualProps.indexOf(expectedProps[i]), -1,
> +        "getOwnPropertyNames should include " + expectedProps[i]);

Maybe we can add this check to dom/tests/mochitest/general/test_interfaces.html?
> * to the left, please

File style disagrees.

> Same here

File style continues to disagree.

> Maybe we can add this check to dom/tests/mochitest/general/test_interfaces.html?

Maybe.  It's not clear what one would check there.

Updated

5 years ago
Attachment #676916 - Flags: review?(jst) → review+
(In reply to Boris Zbarsky (:bz) from comment #3)
> Which may be the same as bug 576449?

Yes, definitely.
(Reporter)

Updated

5 years ago
Duplicate of this bug: 576449
http://hg.mozilla.org/integration/mozilla-inbound/rev/2682f9a1fc58
Flags: in-testsuite+
Whiteboard: [need review][fuzzblocker] → [fuzzblocker]
Target Milestone: --- → mozilla19
Backed out, because b2g is leaking broken turds into the global namespace, but only broken in packaged builds.
Target Milestone: mozilla19 → ---
Created attachment 677963 [details] [diff] [review]
part 1.  Propertly condition MozActivity so we don't stick it on the global in builds that we don't ship the xpt for it in.
Attachment #677963 - Flags: review?(fabrice)
Created attachment 677964 [details] [diff] [review]
part 2.  Condition MozTimeManager classinfo so we don't stick it on the global in builds that don't ship the xpt for it.
Attachment #677964 - Flags: review?(justin.lebar+bug)
Created attachment 677965 [details] [diff] [review]
part 3.  Make sure we enter the right compartment before we try to define interface constants on a constructor.
Attachment #677965 - Flags: review?(bobbyholley+bmo)
Created attachment 677966 [details] [diff] [review]
Part 4: main patch, with an addition test fix because window.O no longer tab-completes unambiguously.
Created attachment 677979 [details] [diff] [review]
part 3.  Make sure we enter the right compartment before we try to define interface constants on a constructor.
Attachment #677979 - Flags: review?(bobbyholley+bmo)
Attachment #677965 - Attachment is obsolete: true
Attachment #677965 - Flags: review?(bobbyholley+bmo)
Attachment #677963 - Flags: review?(fabrice) → review+
I have one remaining test failure: a debug-only timeout in chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_propertyview-reexpand.js

It's pretty easy to reproduce locally, but all it looks like is that the test is just running really slowly.  I'm betting it just can't deal with the larger number of properties, but trying to verify that and see whether it's a problem only for the test or also for the actual debugger.
Created attachment 678011 [details] [diff] [review]
part 5. Fix timeout in debugger propertyview test.

Green on try with this patch. There's some weirdness in OS X 10.6 opt but I believe it's unrelated.

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

* The requestLongerTimeout in browser_dbg_propertyview-reexpand is not necessary afact, but I added it as a reasonable precaution in case this may start timing out again at some point.
* The extra checks for .details.nonenum being expanded were redundant and handled in different tests as well.
* For some reason, with the four patches on win xp debug, I managed to get a timeout just before finishing reload-same-script (test which follows immediately after propertyview-reexpand). I suspect the GC or something else heavy that slows it down; the test finishes correctly if allowed only a few more seconds.
Attachment #678011 - Flags: review?(past)
Attachment #677979 - Flags: review?(bobbyholley+bmo) → review+
Attachment #677964 - Flags: review?(justin.lebar+bug) → review+
Attachment #678011 - Flags: review?(past) → review+
Depends on: 808372
Blocks: 808372
No longer depends on: 808372
Put the debugger fix as part 4, so there are no orange states while bisecting, and pushed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/dc1d29a3f2a7
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8eab1766c2c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9b3b2b8971
   https://hg.mozilla.org/integration/mozilla-inbound/rev/54ab88e2fa34
   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d36471ab3ca
Target Milestone: --- → mozilla19
And backed out part 5 (the actual change) again, since apparently Android is also shipping broken crap.

I'll work on debugging that, I guess; I left the other changesets in for now.
Whiteboard: [fuzzblocker] → [leave open][fuzzblocker]
Created attachment 678602 [details] [diff] [review]
part 5.  Actually ship the xpts for camera api and devicestorage on Firefox for Android, like we do everywhere else.
Attachment #678602 - Flags: review?(khuey)
Attachment #678602 - Flags: review?(khuey) → review+
And with that, try looks like this: https://tbpl.mozilla.org/?tree=Try&rev=ed94ae65e6b1

So I pushed this for what I hope is the last time:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/d9110621e468
   https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8992f602dd
Whiteboard: [leave open][fuzzblocker] → [fuzzblocker]
https://hg.mozilla.org/mozilla-central/rev/dc1d29a3f2a7
https://hg.mozilla.org/mozilla-central/rev/f8eab1766c2c
https://hg.mozilla.org/mozilla-central/rev/0b9b3b2b8971
https://hg.mozilla.org/mozilla-central/rev/54ab88e2fa34
https://hg.mozilla.org/mozilla-central/rev/d9110621e468
https://hg.mozilla.org/mozilla-central/rev/3c8992f602dd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 826658
(Reporter)

Updated

4 years ago
Blocks: 899046
Duplicate of this bug: 687042
Duplicate of this bug: 750104
You need to log in before you can comment on or make changes to this bug.