Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OS 18333466 : Recursion in the generator next call. #5519

Merged
merged 1 commit into from
Jul 25, 2018

Conversation

akroshg
Copy link
Contributor

@akroshg akroshg commented Jul 24, 2018

Moving exception throw out from the catch handler. Otherwise we will throw hard stack overflow.

@akroshg
Copy link
Contributor Author

akroshg commented Jul 24, 2018

@aneeshdk please have a look

{
JavascriptExceptionOperators::DoThrow(exceptionObj, scriptContext);
JavascriptExceptionOperators::DoThrowCheckClone(exception, scriptContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

DoThrowCheckClone [](start = 50, length = 17)

Why does it need to change from DoThrow to DoThrowCheckClone?

Copy link
Contributor

@IrinaYatsenko IrinaYatsenko left a comment

Choose a reason for hiding this comment

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

:shipit:

@MSLaguana
Copy link
Contributor

A similar pattern (a catch with a DoThrow in it) looks like it is in JavascriptOperators::DoIteratorStepAndValue, should we fix that as well?

@akroshg
Copy link
Contributor Author

akroshg commented Jul 24, 2018

@irinayat-MS for OOM and SO we used to clone them as they are static exception. It seems I saw that pattern that's why I changed to that.

@akroshg
Copy link
Contributor Author

akroshg commented Jul 24, 2018

@MSLaguana thanks for finding that out. I'll make that change

@IrinaYatsenko
Copy link
Contributor

Should I do the same in #5497? That one also has special handling for debugging that wraps DoThrow, though...


In reply to: 407584478 [](ancestors = 407584478)

@akroshg
Copy link
Contributor Author

akroshg commented Jul 25, 2018

@irinayat-MS yes - it seems like that is right.

@aneeshdk
Copy link
Contributor

LGTM

Moving exception throw out from the catch handler. Otherwise we will throw hard stack overflow.
Fixed that in the DoIteratorStepAndValue as well
@chakrabot chakrabot merged commit cc098c4 into chakra-core:release/1.10 Jul 25, 2018
chakrabot pushed a commit that referenced this pull request Jul 25, 2018
…call.

Merge pull request #5519 from akroshg:generator

Moving exception throw out from the catch handler. Otherwise we will throw hard stack overflow.
chakrabot pushed a commit that referenced this pull request Jul 25, 2018
…enerator next call.

Merge pull request #5519 from akroshg:generator

Moving exception throw out from the catch handler. Otherwise we will throw hard stack overflow.
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.

5 participants