1377016 - JS_ExtensibleLexicalEnvironment should take unwrapped var object as well as WithEnvironmentObject wrapper

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Philip Chimento [:ptomato]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Steps to reproduce:

JS_ExtensibleLexicalEnvironment() currently requires its argument to be a WithEnvironmentObject wrapper, but it is impossible to create one from embedder code using the JSAPI or friend API.

Background:
===========
I'm trying to upgrade GJS (the GNOME desktop's JS bindings) to use SpiderMonkey ESR52. In GJS we have a "module import" system that predates ES6 modules. Previously we would "import" a module like this:

    JS::AutoObjectVector env_chain(cx);
    env_chain.append(module_obj);
    JS_ExecuteScript(cx, env_chain, module_script, &retval);

Pre-ESR45, any properties defined in module_script, using 'var', 'const', 'let', or 'this.foo = ...', would show up on module_obj. After the lexical environment changes, properties defined with 'let' or 'const' in the script would not show up on module_obj anymore, because they end up in the lexical environment. However, it doesn't seem possible to get access to the lexical environment from embedder code.

Client code of GJS uses 'let' and 'const' all over the place, so this will break extensively. I understand that Firefox went through something similar: https://blog.mozilla.org/addons/2015/10/14/breaking-changes-let-const-firefox-nightly-44/

What I'd like to do, in order to get clients to fix their code, is intercept  property accesses, and if the caller is trying to access a property that's not present on the var-obj but is present on the lexical environment object, then print a warning to the effect of "OK, here is the value of YourModule.foobar, but this is unsupported, please fix your code." (See https://bugzilla.gnome.org/attachment.cgi?id=354606&action=diff for my current attempt at this; ignore the cross-compartment stuff, ideally I'd like to keep everything in the same compartment)

Since there is a 1:1 per-compartment mapping between var-objs and lexical environments, it should be possible to access the lexical environment with JS_ExtensibleLexicalEnvironment(). However, it's currently not possible to create a WithEnvironmentObject using JSAPI, or access the one created in CreateObjectsForEnvironmentChain() (http://searchfox.org/mozilla-central/source/js/src/vm/EnvironmentObject.cpp#3105).


Actual results:

    JS::AutoObjectVector scope_chain(cx);
    scope_chain.append(module);
    if (!JS_ExecuteScript(cx, scope_chain, compiled_script, &retval))
        return false;
    JS::RootedObject lexical_env(cx, JS_ExtensibleLexicalEnvironment(module));
    js::DumpObject(lexical_env);

Assertion failure: lexical, at ~/checkout/mozjs-52.1.2/js/src/jsapi.cpp:1230


Expected results:

No crash, DumpObject shows the properties defined with 'let' and 'const' in the executed script

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8882298 [details] [diff] [review]
Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments.

:ptomato, does this patch work for you?

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments.

evilpie found a stupid bug in getNonSyntacticLexicalEnv. Fixed in this version.

Comment 4

[Philip Chimento [:ptomato]]

Comment on attachment 8882302 [details] [diff] [review]
Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments.

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

Yes, this does exactly what I want when applied to a recent build of ESR52. Thanks!

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8882302 [details] [diff] [review]
Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments.

Cool.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8882302 [details] [diff] [review]
Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a simple semantic change to JSAPI to help GNOME's embedding of SpiderMonkey to not jump through hoops.
User impact if declined: None to Firefox users, but helps maintainability and readability of GNOME code. It's a small enough patch to warrant backporting IMO.
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da7ad37975f1
Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments. (r=jorendorff)

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out on suspicion of frequently asserting at dom/script/ScriptLoader.cpp:649 during wpt instantiation-error-2.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf96c2e5614817945b2e5d8ff19c4c85a75c2c11

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=da7ad37975f1595ac109c3be10af4c2ba6ecd8fa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=116615133&repo=mozilla-inbound

08:47:17     INFO - TEST-START | /html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html
08:47:17     INFO - Setting pref dom.moduleScripts.enabled (true)
08:47:17     INFO - PID 2092 | ++DOCSHELL 23561000 == 7 [pid = 2092] [id = {148c794e-7fc2-4943-8c06-df770d118665}]
08:47:17     INFO - PID 2092 | ++DOMWINDOW == 18 (23561400) [pid = 2092] [serial = 18] [outer = 00000000]
08:47:17     INFO - PID 2092 | ++DOMWINDOW == 19 (23563800) [pid = 2092] [serial = 19] [outer = 23561400]
08:47:17     INFO - PID 2092 | ++DOMWINDOW == 20 (23783C00) [pid = 2092] [serial = 20] [outer = 23561400]
08:47:18     INFO - PID 2092 | Assertion failure: !aRequest->mModuleScript->InstantiationFailed(), at z:/build/build/src/dom/script/ScriptLoader.cpp:649
08:47:18     INFO - PID 2092 | #01: mozilla_dump_image[Z:\task_1500712189\build\application\firefox\xul.dll +0x271623e]
08:47:18     INFO - PID 2092 | #02: mozilla_dump_image[Z:\task_1500712189\build\application\firefox\xul.dll +0x2711f29]
08:47:18     INFO - PID 2092 | #03: ???[Z:\task_1500712189\build\application\firefox\xul.dll +0xf4bd8a]
08:47:18     INFO - PID 2092 | #04: ???[Z:\task_1500712189\build\application\firefox\xul.dll +0xf6af2f]
08:47:18     INFO - PID 2092 | #05: ???[Z:\task_1500712189\build\application\firefox\xul.dll +0xc7e052]
08:47:18     INFO - PID 2092 | #06: ???[Z:\task_1500712189\build\application\firefox\xul.dll +0xc7d3f1]
08:47:18     INFO - PID 2092 | #07: soundtouch::SoundTouch::operator=[Z:\task_1500712189\build\application\firefox\xul.dll +0x1055763]
08:47:18     INFO - PID 2092 | #08: ???[Z:\task_1500712189\build\application\firefox\xul.dll +0x102f1b4]
08:47:18     INFO - PID 2092 | #09: ???[Z:\task_1500712189\build\application\firefox\xul.dll +0x102f16c]
08:47:18     INFO - PID 2092 | #10: ???[Z:\task_1500712189\build\application\firefox\xul.dll +0x102ee95]
08:47:18     INFO - PID 2092 | #11: mozilla_dump_image[Z:\task_1500712189\build\application\firefox\xul.dll +0x277bd03]
08:47:18     INFO - PID 2092 | #12: mozilla_dump_image[Z:\task_1500712189\build\application\firefox\xul.dll +0x27d13a3]
08:47:18     INFO - PID 2092 | #13: workerlz4_maxCompressedSize[Z:\task_1500712189\build\application\firefox\xul.dll +0x39a4904]
08:47:18     INFO - PID 2092 | #14: workerlz4_maxCompressedSize[Z:\task_1500712189\build\application\firefox\xul.dll +0x3a19f32]
08:47:18     INFO - PID 2092 | #15: workerlz4_maxCompressedSize[Z:\task_1500712189\build\application\firefox\xul.dll +0x3a18035]
08:47:18     INFO - PID 2092 | #16: workerlz4_maxCompressedSize[Z:\task_1500712189\build\application\firefox\xul.dll +0x3a17aac]
08:47:18     INFO - PID 2092 | #17: workerlz4_maxCompressedSize[Z:\task_1500712189\build\application\firefox\xul.dll +0x3a23f58]
08:47:18     INFO - PID 2092 | #18: ???[Z:\task_1500712189\build\application\firefox\firefox.exe +0x1767]
08:47:18     INFO - PID 2092 | #19: ???[Z:\task_1500712189\build\application\firefox\firefox.exe +0x13d1]
08:47:18     INFO - PID 2092 | #20: ???[Z:\task_1500712189\build\application\firefox\firefox.exe +0x1aa4]
08:47:18     INFO - PID 2092 | #21: TargetNtUnmapViewOfSection[Z:\task_1500712189\build\application\firefox\firefox.exe +0x36457]
08:47:18     INFO - PID 2092 | #22: BaseThreadInitThunk[C:\windows\system32\kernel32.dll +0x53c45]
08:47:18     INFO - PID 2092 | #23: RtlInitializeExceptionChain[C:\windows\SYSTEM32\ntdll.dll +0x637f5]
08:47:18     INFO - PID 2092 | #24: RtlInitializeExceptionChain[C:\windows\SYSTEM32\ntdll.dll +0x637c8]
08:47:18     INFO - mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/dXfvdHKPSKublotmd1Czqg/artifacts/public/build/target.crashreporter-symbols.zip
08:47:30     INFO - mozcrash Copy/paste: Z:\task_1500712189\build\win32-minidump_stackwalk.exe c:\users\genericworker\appdata\local\temp\tmpnizms8.mozrunner\minidumps\348b0d82-f6e7-48b5-b0f5-bce70251e2f8.dmp c:\users\genericworker\appdata\local\temp\tmp5m52yd
08:47:46     INFO - mozcrash Saved minidump as Z:\task_1500712189\build\blobber_upload_dir\348b0d82-f6e7-48b5-b0f5-bce70251e2f8.dmp
08:47:46     INFO - mozcrash Saved app info as Z:\task_1500712189\build\blobber_upload_dir\348b0d82-f6e7-48b5-b0f5-bce70251e2f8.extra
08:47:46     INFO - PROCESS-CRASH | /html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html | application crashed [@ mozilla::dom::ScriptLoader::StartFetchingModuleDependencies(mozilla::dom::ModuleLoadRequest *)]
08:47:46     INFO - Crash dump filename: c:\users\genericworker\appdata\local\temp\tmpnizms8.mozrunner\minidumps\348b0d82-f6e7-48b5-b0f5-bce70251e2f8.dmp
08:47:46     INFO - Operating system: Windows NT
08:47:46     INFO -                   6.1.7601 Service Pack 1
08:47:46     INFO - CPU: x86
08:47:46     INFO -      GenuineIntel family 6 model 63 stepping 2
08:47:46     INFO -      8 CPUs
08:47:46     INFO - 
08:47:46     INFO - GPU: UNKNOWN
08:47:46     INFO - 
08:47:46     INFO - Crash reason:  EXCEPTION_BREAKPOINT
08:47:46     INFO - Crash address: 0x5768b570
08:47:46     INFO - Process uptime: 6 seconds
08:47:46     INFO - 
08:47:46     INFO - Thread 0 (crashed)
08:47:46     INFO -  0  xul.dll!mozilla::dom::ScriptLoader::StartFetchingModuleDependencies(mozilla::dom::ModuleLoadRequest *) [ScriptLoader.cpp:da7ad37975f1 : 648 + 0x18]
08:47:46     INFO -     eip = 0x5768b570   esp = 0x001fee78   ebp = 0x001fee94   ebx = 0x20055830
08:47:46     INFO -     esi = 0x00000289   edi = 0x2356ed20   eax = 0x00000000   ecx = 0x730306ef
08:47:46     INFO -     edx = 0x00000060   efl = 0x00000216
08:47:46     INFO -     Found by: given as instruction pointer in context
08:47:46     INFO -  1  xul.dll!mozilla::dom::ModuleLoadRequest::ModuleLoaded() [ModuleLoadRequest.cpp:da7ad37975f1 : 73 + 0xf]
08:47:46     INFO -     eip = 0x5768623e   esp = 0x001fee9c   ebp = 0x001feeb0
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO -  2  xul.dll!mozilla::MozPromise<bool,nsresult,0>::InvokeCallbackMethod<0,mozilla::dom::ModuleLoadRequest,void ( mozilla::dom::ModuleLoadRequest::*)(void),bool const &,RefPtr<mozilla::MozPromise<bool,nsresult,0>::Private> >(mozilla::dom::ModuleLoadRequest *,void ( mozilla::dom::ModuleLoadRequest::*)(void),bool const &,RefPtr<mozilla::MozPromise<bool,nsresult,0>::Private> &&) [MozPromise.h:da7ad37975f1 : 564 + 0x9]
08:47:46     INFO -     eip = 0x5767dcec   esp = 0x001feeac   ebp = 0x001feeb0
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO -  3  xul.dll!mozilla::MozPromise<bool,nsresult,0>::ThenValue<mozilla::dom::ModuleLoadRequest *,void ( mozilla::dom::ModuleLoadRequest::*)(void),void ( mozilla::dom::ModuleLoadRequest::*)(void)>::DoResolveOrRejectInternal(mozilla::MozPromise<bool,nsresult,0>::ResolveOrRejectValue &) [MozPromise.h:da7ad37975f1 : 624 + 0x18]
08:47:46     INFO -     eip = 0x57681f29   esp = 0x001feeb8   ebp = 0x001feed0
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO -  4  xul.dll!mozilla::MozPromise<bool,nsresult,0>::ThenValueBase::DoResolveOrReject(mozilla::MozPromise<bool,nsresult,0>::ResolveOrRejectValue &) [MozPromise.h:da7ad37975f1 : 496 + 0xa]
08:47:46     INFO -     eip = 0x55ebbd8a   esp = 0x001feed8   ebp = 0x001feee0
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO -  5  xul.dll!mozilla::MozPromise<bool,nsresult,0>::ThenValueBase::ResolveOrRejectRunnable::Run() [MozPromise.h:da7ad37975f1 : 401 + 0x21]
08:47:46     INFO -     eip = 0x55edaf2f   esp = 0x001feee8   ebp = 0x001feef4
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO -  6  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:da7ad37975f1 : 1579 + 0x11]
08:47:46     INFO -     eip = 0x55bee052   esp = 0x001feefc   ebp = 0x001ff470
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO -  7  xul.dll!NS_ProcessNextEvent(nsIThread *,bool) [nsThreadUtils.cpp:da7ad37975f1 : 530 + 0xd]
08:47:46     INFO -     eip = 0x55bed3f1   esp = 0x001ff478   ebp = 0x001ff48c
08:47:46     INFO -     Found by: previous frame's frame pointer
08:47:46     INFO -  8  xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [MessagePump.cpp:da7ad37975f1 : 97 + 0x8]
08:47:46     INFO -     eip = 0x55fc5763   esp = 0x001ff494   ebp = 0x001ff4b8
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO -  9  xul.dll!MessageLoop::RunInternal() [message_loop.cc:da7ad37975f1 : 321 + 0xf]
08:47:46     INFO -     eip = 0x55f9f1b4   esp = 0x001ff4c0   ebp = 0x001ff4d8
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 10  xul.dll!MessageLoop::RunHandler() [message_loop.cc:da7ad37975f1 : 314 + 0x5]
08:47:46     INFO -     eip = 0x55f9f16c   esp = 0x001ff4e0   ebp = 0x001ff50c
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 11  xul.dll!MessageLoop::Run() [message_loop.cc:da7ad37975f1 : 294 + 0x7]
08:47:46     INFO -     eip = 0x55f9ee95   esp = 0x001ff514   ebp = 0x001ff52c
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 12  xul.dll!nsBaseAppShell::Run() [nsBaseAppShell.cpp:da7ad37975f1 : 156 + 0xc]
08:47:46     INFO -     eip = 0x576ebd03   esp = 0x001ff534   ebp = 0x001ff53c
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 13  xul.dll!nsAppShell::Run() [nsAppShell.cpp:da7ad37975f1 : 271 + 0x8]
08:47:46     INFO -     eip = 0x577413a3   esp = 0x001ff544   ebp = 0x001ff54c
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 14  xul.dll!nsAppStartup::Run() [nsAppStartup.cpp:da7ad37975f1 : 287 + 0x12]
08:47:46     INFO -     eip = 0x58914904   esp = 0x001ff554   ebp = 0x001ff560
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 15  xul.dll!XREMain::XRE_mainRun() [nsAppRunner.cpp:da7ad37975f1 : 4596 + 0x11]
08:47:46     INFO -     eip = 0x58989f32   esp = 0x001ff568   ebp = 0x001ff740
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 16  xul.dll!XREMain::XRE_main(int,char * * const,mozilla::BootstrapConfig const &) [nsAppRunner.cpp:da7ad37975f1 : 4779 + 0x7]
08:47:46     INFO -     eip = 0x58988035   esp = 0x001ff748   ebp = 0x001ff790
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 17  xul.dll!XRE_main(int,char * * const,mozilla::BootstrapConfig const &) [nsAppRunner.cpp:da7ad37975f1 : 4874 + 0x10]
08:47:46     INFO -     eip = 0x58987aac   esp = 0x001ff798   ebp = 0x001ff8a8
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 18  xul.dll!mozilla::BootstrapImpl::XRE_main(int,char * * const,mozilla::BootstrapConfig const &) [Bootstrap.cpp:da7ad37975f1 : 45 + 0xe]
08:47:46     INFO -     eip = 0x58993f58   esp = 0x001ff8b0   ebp = 0x001ff8bc
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 19  firefox.exe!do_main [nsBrowserApp.cpp:da7ad37975f1 : 236 + 0x26]
08:47:46     INFO -     eip = 0x012d1767   esp = 0x001ff8c4   ebp = 0x001ffa00
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 20  firefox.exe!NS_internal_main(int,char * *,char * *) [nsBrowserApp.cpp:da7ad37975f1 : 309 + 0xc]
08:47:46     INFO -     eip = 0x012d13d1   esp = 0x001ffa08   ebp = 0x001ffa38
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 21  firefox.exe!wmain [nsWindowsWMain.cpp:da7ad37975f1 : 115 + 0xf]
08:47:46     INFO -     eip = 0x012d1aa4   esp = 0x001ffa40   ebp = 0x001ffa78
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 22  firefox.exe!__scrt_common_main_seh [exe_common.inl : 253 + 0x1d]
08:47:46     INFO -     eip = 0x01306457   esp = 0x001ffa80   ebp = 0x001ffac0
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 23  kernel32.dll!BaseThreadInitThunk + 0x12
08:47:46     INFO -     eip = 0x769e3c45   esp = 0x001ffac8   ebp = 0x001ffacc
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 24  ntdll.dll!__RtlUserThreadStart + 0x27
08:47:46     INFO -     eip = 0x777237f5   esp = 0x001ffad4   ebp = 0x001ffb0c
08:47:46     INFO -     Found by: call frame info
08:47:46     INFO - 25  ntdll.dll!_RtlUserThreadStart + 0x1b
08:47:46     INFO -     eip = 0x777237c8   esp = 0x001ffb14   ebp = 0x001ffb24
08:47:46     INFO -     Found by: call frame info

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2f9c2b847f
Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments. (r=jorendorff)

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Relanded because the assertion still occurred after the backout, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=116617396&repo=mozilla-inbound

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ed2f9c2b847f

Comment 12

[Shu-yu Guo [:shu]]

Seems like it relanded.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8882302 [details] [diff] [review]
Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments.

Looks like low risk and is needed for spidermonkey. Let's uplift it to ESR52.3.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/319a986ae1f4