Closed Bug 1377016 Opened 7 years ago Closed 7 years ago

JS_ExtensibleLexicalEnvironment should take unwrapped var object as well as WithEnvironmentObject wrapper

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- fixed
firefox56 --- fixed

People

(Reporter: ptomato, Assigned: shu)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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
Flags: needinfo?(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?
Flags: needinfo?(shu)
Attachment #8882298 - Flags: feedback?(philip.chimento)
Assignee: nobody → shu
evilpie found a stupid bug in getNonSyntacticLexicalEnv. Fixed in this version.
Attachment #8882298 - Attachment is obsolete: true
Attachment #8882298 - Flags: feedback?(philip.chimento)
Attachment #8882302 - Flags: feedback?(philip.chimento)
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!
Attachment #8882302 - Flags: feedback?(philip.chimento) → feedback+
Blocks: 1379541
Blocks: sm-embedding
Attachment #8882302 - Flags: review?(jorendorff)
Comment on attachment 8882302 [details] [diff] [review]
Take both with-wrapped and unwrapped enclosing environments when getting non-syntactic lexical environments.

Cool.
Attachment #8882302 - Flags: review?(jorendorff) → review+
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.
Attachment #8882302 - Flags: approval-mozilla-esr52?
Pushed by shu@rfrn.org:
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)
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
Flags: needinfo?(shu)
Pushed by archaeopteryx@coole-files.de:
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)
https://hg.mozilla.org/mozilla-central/rev/ed2f9c2b847f
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Seems like it relanded.
Flags: needinfo?(shu)
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.
Attachment #8882302 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: