Jesse Ruderman Reporter
	Description • 12 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 ?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 12 years ago

Looks like the fundamental issue is that nsWindowSH::Enumerate only enumerates standard classes, and not all the things from the namespace manager, right?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 12 years ago

Which may be the same as bug 576449?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 12 years ago

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...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 12 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 8 • 12 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 12 years ago

> * 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.

	Jeff Walden [:Waldo]
	Comment 10 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #3)
> Which may be the same as bug 576449?

Yes, definitely.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/2682f9a1fc58

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 12 years ago

Backed out, because b2g is leaking broken turds into the global namespace, but only broken in packaged builds.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 12 years ago

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.

	Victor Porof [:vporof][:vp]
	Comment 20 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 12 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 25 • 12 years ago

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