Summary: | Assertions in JSC::StackVisitor::Frame::existingArguments() during stack unwinding | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Akos Kiss <akiss> | ||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, buildbot, commit-queue, fpizlo, ggaren, mark.lam, oliver, rniwa | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Akos Kiss
2014-11-09 06:23:17 PST
Created attachment 241252 [details]
Proposed patch.
The patch fixes both assertions and causes no jsc test regressions.
Comment on attachment 241252 [details]
Proposed patch.
Can you write a test for this?
Comment on attachment 241252 [details]
Proposed patch.
When codeBlock->needsActivation() is true, how is it possible that the frame lacks an activation? I believe that should be impossible.
If I'm right, create_lexical_environment at [3] sets up activation to r-1. So, before that, although codeBlock()->needsActivation() is true, the frame does not have activation yet. About the test: I'm not sure yet that I can force the first 6 instructions throw an exception... at least "naturally", but only from "outside" by ExceptionFuzz. When ExceptionFuzz was introduced in http://trac.webkit.org/changeset/171213 , StackVisitor was modified to handle throws in op_enter. I guess that was also only to cover such artificial fuzz exceptions. (At least I could not find any already existing test that would reliably trigger that exception, unfortunately.) Looks like this has been failing on the bots periodically: <rdar://problem/18867723>. I see: this failure is only possible in fuzzing mode, which sometimes inserts exceptions in places where they would otherwise be impossible. Comment on attachment 241252 [details] Proposed patch. It feels a bit awkward to program defensively like this just to make the fuzzer happy. Programming like this means that we can't tell the difference between "Something is seriously wrong because the activation object is missing" and "I'm just fuzzing". Ideally, we would teach the fuzzer not to throw in cases that otherwise couldn't -- for example, in the LLInt, by passing an argument to END() that said "ASSERT there is no exception, and do not fuzz for exceptions". I guess this patch is an improvement for now, so it's worth doing. Note, though that you missed a spot: Oliver removed the original work-around for fuzzing, probably because he wasn't aware of this special fuzzing behavior. You should update your comments to specify that we do this only for fuzzing, and also add back the code that Oliver removed in <http://trac.webkit.org/changeset/174226>: Index: /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp =================================================================== --- /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp (revision 174225) +++ /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp (revision 174226) @@ -440,6 +440,4 @@ CallFrame* callFrame = visitor->callFrame(); CodeBlock* codeBlock = visitor->codeBlock(); - JSScope* scope = callFrame->scope(); - if (Debugger* debugger = callFrame->vmEntryGlobalObject()->debugger()) { ClearExceptionScope scope(&callFrame->vm()); @@ -456,13 +454,4 @@ RELEASE_ASSERT(!visitor->isInlinedFrame()); #endif - lexicalEnvironment = callFrame->uncheckedActivation(); - // Protect against the lexical environment not being created, or the variable still being - // initialized to Undefined inside op_enter. - if (lexicalEnvironment && lexicalEnvironment.isCell()) { - JSLexicalEnvironment* activationObject = jsCast<JSLexicalEnvironment*>(lexicalEnvironment); - // Protect against throwing exceptions after tear-off. - if (!activationObject->isTornOff()) - activationObject->tearOff(*scope->vm()); - } } (In reply to comment #7) > Comment on attachment 241252 [details] > Proposed patch. > > It feels a bit awkward to program defensively like this just to make the > fuzzer happy. Programming like this means that we can't tell the difference > between "Something is seriously wrong because the activation object is > missing" and "I'm just fuzzing". > > Ideally, we would teach the fuzzer not to throw in cases that otherwise > couldn't -- for example, in the LLInt, by passing an argument to END() that > said "ASSERT there is no exception, and do not fuzz for exceptions". > > I guess this patch is an improvement for now, so it's worth doing. Note, > though that you missed a spot: Oliver removed the original work-around for > fuzzing, probably because he wasn't aware of this special fuzzing behavior. > > You should update your comments to specify that we do this only for fuzzing, > and also add back the code that Oliver removed in > <http://trac.webkit.org/changeset/174226>: > > Index: /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp > =================================================================== > --- /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp (revision > 174225) > +++ /trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp (revision > 174226) > @@ -440,6 +440,4 @@ > CallFrame* callFrame = visitor->callFrame(); > CodeBlock* codeBlock = visitor->codeBlock(); > - JSScope* scope = callFrame->scope(); > - > if (Debugger* debugger = callFrame->vmEntryGlobalObject()->debugger()) { > ClearExceptionScope scope(&callFrame->vm()); > @@ -456,13 +454,4 @@ > RELEASE_ASSERT(!visitor->isInlinedFrame()); > #endif > - lexicalEnvironment = callFrame->uncheckedActivation(); > - // Protect against the lexical environment not being created, or > the variable still being > - // initialized to Undefined inside op_enter. > - if (lexicalEnvironment && lexicalEnvironment.isCell()) { > - JSLexicalEnvironment* activationObject = > jsCast<JSLexicalEnvironment*>(lexicalEnvironment); > - // Protect against throwing exceptions after tear-off. > - if (!activationObject->isTornOff()) > - activationObject->tearOff(*scope->vm()); > - } > } For what it's worth, when we added the fuzzer we added a handful of such checks that are only to defend against the fuzzer. This involved far less infrastructure than making the fuzzer more complicated. Also, if we ever were wrong about our assumptions about when activations are allocated or who could throw exceptions, this defensiveness would turn a crash into something much less severe. Created attachment 241354 [details]
Updated patch
Updated/added comments as requested.
However, I don't think that the change to Interpreter.cpp could/should be reverted. The code that was removed there was callinf isTornOff and tearOff on LexicalEnvironment. However, that API/functionality does not exists anymore.
> However, I don't think that the change to Interpreter.cpp could/should be
> reverted. The code that was removed there was callinf isTornOff and tearOff
> on LexicalEnvironment. However, that API/functionality does not exists
> anymore.
Aha! You are correct.
Comment on attachment 241354 [details]
Updated patch
r=me
Comment on attachment 241354 [details] Updated patch Clearing flags on attachment: 241354 Committed r175967: <http://trac.webkit.org/changeset/175967> All reviewed patches have been landed. Closing bug. |