Closed
Bug 986767
Opened 10 years ago
Closed 10 years ago
Fix adjusting stepModeCount when removing a debuggee global from inside the onStep handler
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: shu, Assigned: shu)
References
Details
Attachments
(1 file, 3 obsolete files)
It is allowed to remove debuggee globals from inside the onStep handler. Currently this causes the stepModeCount to get into an inconsistent state.
Assignee | ||
Comment 1•10 years ago
|
||
Test for this is 'resumption-08.js' over in the tests part of bug 716647.
Attachment #8395139 -
Flags: review?(jimb)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8400146 -
Flags: review?(jimb)
Assignee | ||
Updated•10 years ago
|
Attachment #8395139 -
Attachment is obsolete: true
Attachment #8395139 -
Flags: review?(jimb)
Updated•10 years ago
|
Attachment #8400146 -
Flags: review?(jimb)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8400146 -
Attachment is obsolete: true
Attachment #8400289 -
Flags: review?(jimb)
Assignee | ||
Comment 4•10 years ago
|
||
Had a typo, also testing git-bz
Attachment #8400289 -
Attachment is obsolete: true
Attachment #8400289 -
Flags: review?(jimb)
Attachment #8400291 -
Flags: review?(jimb)
Comment 5•10 years ago
|
||
Comment on attachment 8400291 [details] [diff] [review] Fix adjusting stepModeCount when removing a debuggee global from inside the onStep handler. Review of attachment 8400291 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with comments addressed. Is there no way to test that this fix works? ::: js/src/jsscript.cpp @@ +3101,5 @@ > return true; > } > > void > +JSScript::setNewStepMode(FreeOp *fop, uint32_t newValue) It doesn't seem like 'fop' is used. Should we be using it to free the debugScript? @@ +3107,5 @@ > DebugScript *debug = debugScript(); > uint32_t prior = debug->stepMode; > debug->stepMode = newValue; > > + if (prior != newValue) { Changing the comparison as booleans to a comparison as ints is a bug, you said on IRC. @@ +3156,2 @@ > DebugScript *debug = debugScript(); > uint32_t count = debug->stepMode & stepCountMask; Should we assert that count > 0? ::: js/src/jsscript.h @@ +1484,1 @@ > /* Attempt to change this->stepMode to |newValue|. */ Since it's infallible, "Attempt" is no longer accurate, right? ::: js/src/vm/Debugger.cpp @@ +2274,5 @@ > GlobalObjectSet::Enum *compartmentEnum, > GlobalObjectSet::Enum *debugEnum) > { > AutoDebugModeInvalidation invalidate(global->compartment()); > + removeDebuggeeGlobal(fop, global, invalidate, compartmentEnum, debugEnum); [stares] Thanks :( @@ +2310,3 @@ > if (&frame.script()->global() == global) { > + DebuggerFrame_freeScriptFrameIterData(fop, frameobj); > + DebuggerFrame_maybeDecrementFrameScriptStepModeCount(fop, frame, frameobj); So, this bit is the actual bug fix, right?
Attachment #8400291 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Jim Blandy :jimb from comment #5) > Comment on attachment 8400291 [details] [diff] [review] > Fix adjusting stepModeCount when removing a debuggee global from inside the > onStep handler. > > Review of attachment 8400291 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, with comments addressed. > > Is there no way to test that this fix works? > The test is resumption-08 over in bug 716647. I don't know how to test this without the OSR patches.
Comment 7•10 years ago
|
||
As long as the OSR patches make resumption-08 fail, that's fine. I'd forgotten the details.
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1354a3e748e
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a1354a3e748e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•