...
Created attachment 271119 [details] patch
Comment on attachment 271119 [details] patch IIRC correctly the purpose of this logic wasn't to be correct wrt language semantics, just correct-enough to ensure that we didn't accidentally try to capture from the global object. It was essentially a safety thing for builtins.
(In reply to comment #2) > Comment on attachment 271119 [details] > patch > > IIRC correctly the purpose of this logic wasn't to be correct wrt language > semantics, just correct-enough to ensure that we didn't accidentally try to > capture from the global object. It was essentially a safety thing for > builtins. Which logic are you talking about?
I think you're probably talking about this loop: - if (!capturedVariables.isEmpty()) { - for (const auto& capturedVariable : capturedVariables) { - if (scope->hasDeclaredVariable(capturedVariable)) - continue; - - if (scope->hasDeclaredParameter(capturedVariable)) - continue; - - RELEASE_ASSERT_NOT_REACHED(); - } - } And if so, we want the logic to be something like: ``` if (m_parsingBuiltin) RELEASE_ASSERT(capturedVariableCandidates().isEmpty()); ``` Out of curiosity, why do we enforce this? Is it just that global properties can be changed by user code?
(In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 271119 [details] > > patch > > > > IIRC correctly the purpose of this logic wasn't to be correct wrt language > > semantics, just correct-enough to ensure that we didn't accidentally try to > > capture from the global object. It was essentially a safety thing for > > builtins. > > Which logic are you talking about? Literally the entirety of what you just removed. Again, this is very much an "IIRC", and all I can recall is that the particular set of rules it ended up enforcing were those necessary to disallow capturing global variables. I agree the mismatched naming vs. semantics warrant fixing, but with this change it becomes possible to accidentally capture unprivileged global variables, e.g. Object vs @Object. Given the testing we could also change the logic that achieves this to be at bytecode generation time to fail out on any non-local resolve (which is probably a more sensible way to do it).
(In reply to comment #4) > I think you're probably talking about this loop: > - if (!capturedVariables.isEmpty()) { > - for (const auto& capturedVariable : capturedVariables) { > - if (scope->hasDeclaredVariable(capturedVariable)) > - continue; > - > - if (scope->hasDeclaredParameter(capturedVariable)) > - continue; > - > - RELEASE_ASSERT_NOT_REACHED(); > - } > - } > > And if so, we want the logic to be something like: > ``` > if (m_parsingBuiltin) > RELEASE_ASSERT(capturedVariableCandidates().isEmpty()); > ``` > > Out of curiosity, why do we enforce this? Is it just that > global properties can be changed by user code? Yup, which (among other things) leads to sadness in language semantics, as well as potential security issues.
(In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #2) > > > Comment on attachment 271119 [details] > > > patch > > > > > > IIRC correctly the purpose of this logic wasn't to be correct wrt language > > > semantics, just correct-enough to ensure that we didn't accidentally try to > > > capture from the global object. It was essentially a safety thing for > > > builtins. > > > > Which logic are you talking about? > > Literally the entirety of what you just removed. > > Again, this is very much an "IIRC", and all I can recall is that the > particular set of rules it ended up enforcing were those necessary to > disallow capturing global variables. I agree the mismatched naming vs. > semantics warrant fixing, but with this change it becomes possible to > accidentally capture unprivileged global variables, e.g. Object vs @Object. > > Given the testing we could also change the logic that achieves this to be at > bytecode generation time to fail out on any non-local resolve (which is > probably a more sensible way to do it). Okay. I agree that we should enforce this. I was just observing when reading the code that the code doesn't do anything. We need to come up with some rules that work. I don't think this can be done from the BytecodeGenerator. It would be possible to do during Bytecode linking, though. I think solving this in the parser still might be easiest. I think the logic would go something like this. ``` after parsing is complete, do: if (weAreParsingAProgram()) { if (captureVariableCandidates() \ definedVariables() != emptySet) crash() } ```
Comment on attachment 271119 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271119&action=review > Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:-115 > - for (const auto& closedVariable : program->closedVariables()) { > - if (closedVariable == vm.propertyNames->arguments.impl()) > - continue; > - > - if (closedVariable == vm.propertyNames->undefinedKeyword.impl()) > - continue; > - } Oliver, do you remember why we had this here? Instead of strictly being in the parser? This is a very curious loop, it looks like it's probably been edited to death.
I wonder if we have any errors that have slipped through here.
(In reply to comment #9) > I wonder if we have any errors that have slipped through here. Yup. We totally do!
(In reply to comment #10) > (In reply to comment #9) > > I wonder if we have any errors that have slipped through here. > > Yup. We totally do! Oh man, we get it wrong in so many places :/
Created attachment 271238 [details] patch
Attachment 271238 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/parser/Parser.cpp:304: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/parser/Parser.cpp:305: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 271239 [details] patch
Attachment 271239 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 271239 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271239&action=review r=me > Source/JavaScriptCore/parser/Parser.cpp:334 > + if (!lexicalVariables.contains(candidate) && !varDeclarations.contains(candidate) && !builtinNames.isPrivateName(*candidate.get())) { > + dataLog("Bad global capture in builtin: '", candidate, "'\n"); > + dataLog(m_source->view()); > + CRASH(); > } Can we make this an ASSERT instead?
Comment on attachment 271239 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271239&action=review > Source/JavaScriptCore/parser/Parser.cpp:302 > + scope->getCapturedVars(capturedVariables, modifiedParameter, modifiedArguments); // FIXME: make this just find out modifedParamerer/modifiedArguments Maybe you should file a bug for this (and include the bug url here)?
Comment on attachment 271239 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271239&action=review >> Source/JavaScriptCore/parser/Parser.cpp:302 >> + scope->getCapturedVars(capturedVariables, modifiedParameter, modifiedArguments); // FIXME: make this just find out modifedParamerer/modifiedArguments > > Maybe you should file a bug for this (and include the bug url here)? oops, this FIXME should just be removed. I was anticipating rewriting this function but I didn't
Comment on attachment 271239 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271239&action=review >> Source/JavaScriptCore/parser/Parser.cpp:334 >> } > > Can we make this an ASSERT instead? I think this reporting is actually very helpful. I'm going to keep it. I'm going to put the whole loop under #ifndef NDEBUG
landed in: http://trac.webkit.org/changeset/196525