Closed Bug 807222 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(Whiteboard: [fuzzblocker])

Attachments

(8 files, 1 obsolete file)

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+bug
: review+
Details | Diff | Splinter Review
7.69 KB, patch
Details | Diff | Splinter Review
948 bytes, patch
bholley
: 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
(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?
Attached file 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.
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.
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.
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 → ---
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.
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
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]
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]
Duplicate of this bug: 826658
Blocks: 899046
Duplicate of this bug: 687042
Duplicate of this bug: 750104
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.