1406146 - Support WindowProxy in jsshell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Ted Campbell [:tcampbell]]

While doing cleanup I noticed that we don't appear to have a way to have a global with a WindowProxy while running in the shell. As we keep optimizing WindowProxy perf, we should probably have a way to do targeted tests in the shell and flush out little bugs.

I propose adding an option to the newGlobal() shell function that creates the global with a dummy WindowProxy. It doesn't have to do anything special for now.

Comment 1

[Ted Campbell [:tcampbell]]

Some good points made by :waldo that emulating Window behavior is doomed to bit-rot. The hope here is to avoid navigation and other complexities and just have a transparent proxy.

This would still allow us to test parts of the JIT that see a WindowProxy as the global |this|. We have various ICs that try to optimize for them so it would be nice to have shell tests for them.

Hopefully this small scope is feasible.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1406146-Support-a-simple-WindowProxy-in-the-jssh.patch

Simple WindowProxy class for shell testing. New globals can opt in.

Asking :waldo for f? since he expressed concerns about false positive bug reports.

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: 0002-Bug-1406146-Fix-DebuggerObject-unsafeDereference-han.patch

Debugger interface trips assertions and leaks unwrapped Windows when using unsafeDereference.

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: 0003-Bug-1406146-Add-jsshell-test-for-WindowProxy-ICs.patch

Add shell tests to CacheIR for some WindowProxy cases.

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: 0004-NOMERGE-Fixup-debugger-tests-for-WindowProxy-jsshell.patch

These are fixes where debugger tests were imprecise and relied on no WindowProxy existing.

Jason, does it make sense to apply these changes, or do we leave tests as is? They continue to pass since WindowProxy is opt-in.

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: 0005-NOMERGE-Add-Proxy-watch-handler-for-ShellProxyWindow.patch

While testing the hypothetical case of WindowProxy for all shell globals, the remain test failures were do to missing watch/unwatch proxy hooks. This patch is just here for documentation. There is talk about removing watch/unwatch from the codebase anyways.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8917102 [details] [diff] [review]
0001-Bug-1406146-Support-a-simple-WindowProxy-in-the-jssh.patch

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

::: js/src/shell/js.cpp
@@ +8674,5 @@
>      JS_SetTrustedPrincipals(cx, &ShellPrincipals::fullyTrusted);
>      JS_SetSecurityCallbacks(cx, &ShellPrincipals::securityCallbacks);
>      JS_InitDestroyPrincipalsCallback(cx, ShellPrincipals::destroy);
>  
> +    js::SetWindowProxyClass(cx, &ShellWindowProxyClass);

Drive-by nit: you probably want to add this also next to JS_InitDestroyPrincipalsCallback in WorkerMain, so it works with evalInWorker() too.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8917104 [details] [diff] [review]
0003-Bug-1406146-Add-jsshell-test-for-WindowProxy-ICs.patch

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

Excellent, thanks.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8917102 [details] [diff] [review]
0001-Bug-1406146-Support-a-simple-WindowProxy-in-the-jssh.patch

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

This is not particularly my strong suit, to be honest, so I'm not sure how much any comment on this is worth.  The + is mostly a matter of "I've looked at it, here are Things" than "this is a stonking good idea".

The original split-stuff was removed in bug 676708.  As you'll see if you look, it was much more ponderous than any of this is.  That seems more promising for the survivability of this, if it lands.

Now, for comparing the old stuff to the new stuff -- are there any features of the old code that would be useful to have here?  Like, the property that exists only on the inner, to distinguish it if it escapes to code?  (Maybe, I think?)  Or the standard classes being on both objects?  (No, I think?)  Mostly just throwing out comments/questions to which I don't know if I have answers -- ask more people, get more answers, etc.

::: js/src/builtin/TestingFunctions.cpp
@@ +3568,5 @@
>      if (!JS::CompileForNonSyntacticScope(cx, options, srcBuf, &script))
>          return false;
>  
>      if (global) {
> +        global = CheckedUnwrap(global, /* stopAtWindowProxy = */false);

General style would have one space after the comment-clone.

::: js/src/shell/js.cpp
@@ +317,5 @@
> +    "ShellWindowProxyClass",
> +    JSCLASS_HAS_RESERVED_SLOTS(1));
> +
> +JSObject *
> +NewShellWindowProxy(JSContext* cx, JS::HandleObject global) {

Brace for function bodies goes on its own line.

@@ +6596,5 @@
>  "  Return a new global object in a new compartment. If options\n"
>  "  is given, it may have any of the following properties:\n"
>  "      sameZoneAs: the compartment will be in the same zone as the given object (defaults to a new zone)\n"
>  "      invisibleToDebugger: the global will be invisible to the debugger (default false)\n"
> +"      useWindowProxy: the global will be created with a WindowProxy attached\n"

Possibly worth saying the WindowProxy will be returned instead of the global.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8917102 [details] [diff] [review]
0001-Bug-1406146-Support-a-simple-WindowProxy-in-the-jssh.patch

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

::: js/src/shell/js.cpp
@@ +305,5 @@
>  CancelExecution(JSContext* cx);
>  
>  static JSObject*
>  NewGlobalObject(JSContext* cx, JS::CompartmentOptions& options,
> +                JSPrincipals* principals, bool useWindowProxy = false);

Enum instead of bool here, please.

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8917107 [details] [diff] [review]
0005-NOMERGE-Add-Proxy-watch-handler-for-ShellProxyWindow.patch

Hiding this now that Bug 638054 has landed.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8917103 [details] [diff] [review]
0002-Bug-1406146-Fix-DebuggerObject-unsafeDereference-han.patch

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

I'm so confused. :D  Sorry if these points are nonsense.

- What assertions trip if we leave this alone?

- Changing this API is not a slam dunk for me. The point of the Debugger is to see through some abstractions, exposing the building blocks of the language's actual behavior, including things like the global. Not to say our devtools actually care, but it's possible they do.

- Even assuming we really mustn't expose the global, even to Debugger users, it seems iffy to change this in this particular way. I don't have a better idea... but the Window and WindowProxy are clearly distinct objects; silently substituting one for the other is awfully subtle.

- Assuming it is a good idea, I still have a nit: the C++ code being added here is too abstract. If this is strictly meant to susbtitute the WindowProxy for the Window, surely there's a more direct way of saying that.

Comment 13

[Ted Campbell [:tcampbell]]

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> Comment on attachment 8917103 [details] [diff] [review]
> 0002-Bug-1406146-Fix-DebuggerObject-unsafeDereference-han.patch
> 
> Review of attachment 8917103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm so confused. :D  Sorry if these points are nonsense.
> 
> - What assertions trip if we leave this alone?
> 

The failing assertion is immediately following. Problem was never triggered in shell, but assertion trips in browser or xpcshell.

https://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/js/src/vm/Debugger.cpp#10716-10717

Comment 14

[Jason Orendorff [:jorendorff]]

The assertion was added in bug 916321, r=me four years ago.

I'm still not clear on why the assertion is not tripping all the time already, or why the new C++ code works.

It's bad that double wrapping has any effect at all.

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> I'm still not clear on why the assertion is not tripping all the time
> already, or why the new C++ code works.

...Or why it doesn't trigger in the shell, with the new split globals.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 8917105 [details] [diff] [review]
0004-NOMERGE-Fixup-debugger-tests-for-WindowProxy-jsshell.patch

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

Since we do not have comprehensive Debugger tests that run in the browser, by far the best thing would be to change (most of) these tests to use globals that have a WindowProxy.

Comment 17

[Ted Campbell [:tcampbell]]

Following up with some notes from IRC. It seems the problem with my patch versus Gecko is that we currently rely on the preWrap hook to add WindowProxy in the cross-compartment case. I put together a try run where we add the WindowProxy under JSCompartment::getNonWrapperObjectForCurrentCompartment instead.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=780cb95dd277060bda6ff64921f8a110e362ced2

This might be clearer than patch 2's approach. My comments should be probably be improved.

Comment 18

[Ted Campbell [:tcampbell]]

That experiment did not end well. The xpconnect wrapper factory really does want the bare Window or assertions start failing. There are complexities about CCWs, DeadProxy, and ExposeObjectToActiveJS that would require further work. This is a quagmire that I am not well versed about.

Perhaps I need a preWrap hook for the ShellWindowProxy that simply handles ToWindowProxyIfWindow.

Comment 19

[Ted Campbell [:tcampbell]]

As suggested by :bz, it might make sense to hoist http://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/js/xpconnect/wrappers/WrapperFactory.cpp#164-181 from xpconnect into the js engine now that WindowProxy is an JS engine concept.

Comment 20

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1406146-Support-a-simple-WindowProxy-in-the-jssh.patch

Refreshed with patch. Now with an enum instead of a boolean as argument when creating a global in shell.

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] from comment #19)

it might make sense to hoist
http://searchfox.org/mozilla-central/rev/
423b2522c48e1d654e30ffc337164d677f934ec3/js/xpconnect/wrappers/
WrapperFactory.cpp#164-181 from xpconnect into the js engine now that
WindowProxy is an JS engine concept.

I'm doing exactly that in bug 1534902, for what it's worth.

Comment 22

[Ted Campbell [:tcampbell]]

Great. I've rebased locally and am sorting out a few failures but I think it should be possible to finish this now that Bug 1534902 is landed.

Comment 23

[Ted Campbell [:tcampbell]]

Attachment: Bug 1406146 - Fix handling of WindowProxy in jsshell testing functions. r?jandem

This patch fixes a few cases in testing functions that do not correctly
handling WindowProxy wrappers. This mostly involves passing the
stopAtWindowProxy=false argument to unwrap functions.

Comment 24

[Ted Campbell [:tcampbell]]

Attachment: Bug 1406146 - Support a simple WindowProxy in the jsshell. r?jandem

This adds an option to the 'newGlobal' shell command to create globals
that have a simple WindowProxy. This is intended for testing code that
distinguishes between GlobalObject and WindowProxy. No typical window
behavior such as navigation will be supported.

Depends on D26836

Comment 25

[Ted Campbell [:tcampbell]]

Attachment: Bug 1406146 - Add jsshell test for WindowProxy ICs. r?jandem

Depends on D26837

Comment 26

[Ted Campbell [:tcampbell]]

Attachment: Bug 1406146 - Fix debugger tests under WindowProxy. r?jorendorff

Fix various tests that relied on the shell global being bare and not
having a WindowProxy. These tests should now pass both with and without
a WindowProxy.

Depends on D26838

Comment 27

[Ted Campbell [:tcampbell]]

Attachment: Bug 1406146 - Use ShellWindowProxy in jsshell by default. r?jandem,jorendorff

Depends on D26839

Comment 28

[Ted Campbell [:tcampbell]]

Comment on attachment 9057133 [details]
Bug 1406146 - Fix debugger tests under WindowProxy. r?jorendorff

Yoric, can you generate a patch for the binast variant of this patch? The binjs-ref tool simply panics when I run it with a generic error about 'Error in dedicated thread: Any'.

Alternately, can I just remove the failing tests for now until the next time they get updated?

Comment 29

[Ted Campbell [:tcampbell]]

Attachment: Bug 1406146 - Fix cross-compartment global issue in typedarary test. r?jandem

This is an artifact of the same-compartment realm changes. The test
confuses CCWs and WindowProxies.

Comment 30

[Ted Campbell [:tcampbell]]

Studying the situation more, it seems that deleting tests is the standard fix for the binjs problem and we aren't regularly updating them anyways. I've filed Bug 1543375 to suggest we add binjs-ref to the tooltool repository.

I'll post a patch to delete those tests.

Comment 31

[Ted Campbell [:tcampbell]]

Attachment: Bug 1406146 - Remove modified debugger jit-tests from BinAST suite. r?arai

These tests use our non-standard Debugger interface and there are minor
changes needed for them to run in the case of WindowProxy in shell.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd75d6285f74
Fix handling of WindowProxy in jsshell testing functions. r=jandem
https://hg.mozilla.org/integration/autoland/rev/dc43810c71fd
Support a simple WindowProxy in the jsshell. r=jandem
https://hg.mozilla.org/integration/autoland/rev/32cbcac2b40a
Add jsshell test for WindowProxy ICs. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c64199fb6020
Fix cross-compartment global issue in typedarary test. r=jandem

Comment 33

[Ted Campbell [:tcampbell]]

(Pushed the opt-in support for WindowProxy in shell. Once we agree on the test changes and look at perf, we can turn it on by default for the shell)

TODO: Add a testcase that the option actually works by using isWrapper(this)

Comment 34

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/fd75d6285f74
https://hg.mozilla.org/mozilla-central/rev/dc43810c71fd
https://hg.mozilla.org/mozilla-central/rev/32cbcac2b40a
https://hg.mozilla.org/mozilla-central/rev/c64199fb6020

Comment 35

[Ted Campbell [:tcampbell]]

When running octane with and without changing the defaults, things are all within noise margin except for CodeLoad which slows down 5% by having WindowProxy.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13a6fa6a0b9f
Fix debugger tests under WindowProxy. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/badb8ef128dd
Remove modified debugger jit-tests from BinAST suite. r=arai
https://hg.mozilla.org/integration/autoland/rev/2b2554ff8f8c
Use ShellWindowProxy in jsshell by default. r=jandem,jorendorff

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/13a6fa6a0b9f
https://hg.mozilla.org/mozilla-central/rev/badb8ef128dd
https://hg.mozilla.org/mozilla-central/rev/2b2554ff8f8c

Comment 38

[Ted Campbell [:tcampbell]]

(Planned work is now landed. That regression still needs to be addressed in that bug itself).