BUG 8258993: Remove Assert that is causing many hits on the OOM path because of inconsistent finalization flags state. #1362
Conversation
|
@Yongqu review please? |
| @@ -375,7 +375,7 @@ namespace Js | |||
|
|
|||
| ScriptContext::~ScriptContext() | |||
| { | |||
| Assert(isFinalized || !isInitialized); | |||
| Assert(isFinalized); | |||
There was a problem hiding this comment.
shouldn't revert. We can get rid of the whole assert.
I was afraid we might miss some scenario where we would delete scriptContext is wrong time. I think we cover all three now and the assert can be removed:
. finalize time when the scriptcontext is held by library
. half way during initialization of scriptcontext when we OOM; in both browser & JSRT scenario
. JSRT only, fail half way during JSRTContext initialization, but after scriptcontext is fully created.
There was a problem hiding this comment.
Logically isFinalized || !isInitialized seems like a reasonable assert though. Maybe we should fix the state of those flags along error paths so that we could keep the assert and ensure that we've really handled all of the cases...
There was a problem hiding this comment.
then we need another state to know that we are half way in JsrtContext initialization code path...
There was a problem hiding this comment.
Ah I see. Maybe it's not worth doing that bookkeeping? In which case, deleting this assert seems like the right thing to do I guess.
There was a problem hiding this comment.
yeah I think we should remove the assert at this time.
dilijev
changed the title
|
LGTM |
…because of inconsistent finalization flags state. Reviewed by @Yongqu
|
@dotnet-bot test Windows x64_release please |
Reviewed by @Yongqu