Skip to content

Conversation

@atulkatti
Copy link
Contributor

@atulkatti atulkatti commented Jan 4, 2017

In defer parse cases, sometimes byte code emitter seems to aggressively process nested scopes that the byte code generator hasn't visited. This results in assertion failures as certain flags are not set as expected. This surfaces more in cases where parameter scope has a class declaration and either the class declaration has an eval or the enclosing function is in an eval/global eval.

@msftclas
Copy link

msftclas commented Jan 4, 2017

Hi @atulkatti, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Atul Katti). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@atulkatti atulkatti changed the title Resolves Bug#8971506. Defer parse case, assertions due to ByteCodeEmitter aggressively emitting nodes unprocessed yet by ByeCodeGenerator. Resolves Bug#8971506. Defer parse case, assertions due to ByteCodeEmitter aggressively emitting nodes unprocessed yet by ByteCodeGenerator. Jan 4, 2017
@atulkatti atulkatti requested review from aneeshdk, suwc and tcare and removed request for suwc January 4, 2017 06:19
this->EmitOneFunction(pnode);
this->EndEmitFunction(pnode);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space here.

@atulkatti atulkatti requested a review from suwc January 4, 2017 06:22
@atulkatti
Copy link
Contributor Author

@Microsoft/chakra-developers

newByteLength = mappedLength * elementSize;

if ((uint32)mappedLength > (byteLength - offset)/ elementSize)
if (offset + newByteLength > byteLength)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these and many other changes in this PR part of #2289?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Please only look at the last commit with message "Resolves Bug#8971506...". The other commits are a result of a merge I did after pushing my branch. But those commits are already merged to release/1.4 branch. Sorry about the confusion.

@aneeshdk
Copy link
Contributor

aneeshdk commented Jan 4, 2017

LGTM

@tcare
Copy link
Contributor

tcare commented Jan 4, 2017

Looks good

…tter aggressively emitting nodes unprocessed yet by ByteCodeGenerator.
@pleath
Copy link
Contributor

pleath commented Jan 5, 2017

The symptom you describe puzzles me. Can you may out an example of how this happens?

Copy link
Contributor

@dilijev dilijev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but wait for someone with better knowledge of the parser to sign off.

(eval(`
function f7(a3 = class c4 extends false {
}) { };
`));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the extra set of parens around the eval(...) necessary to set up the scopes correctly to repro the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the extra set of parenthesis aren't necessary.

@ThomsonTan
Copy link
Collaborator

Could the similar problem happen in other places?

@atulkatti atulkatti closed this Jan 30, 2017
@atulkatti
Copy link
Contributor Author

A more appropriate fix for this bug is to remove the isEnclosedInParamScope flag. Once that happens this fix will not be required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants