Closed Bug 986767 Opened 6 years ago Closed 5 years ago

Fix adjusting stepModeCount when removing a debuggee global from inside the onStep handler

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

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.
Test for this is 'resumption-08.js' over in the tests part of bug 716647.
Attachment #8395139 - Flags: review?(jimb)
Blocks: 716647
Attachment #8395139 - Attachment is obsolete: true
Attachment #8395139 - Flags: review?(jimb)
Attachment #8400146 - Flags: review?(jimb)
Had a typo, also testing git-bz
Attachment #8400289 - Attachment is obsolete: true
Attachment #8400289 - Flags: review?(jimb)
Attachment #8400291 - Flags: review?(jimb)
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+
(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.
As long as the OSR patches make resumption-08 fail, that's fine. I'd forgotten the details.
https://hg.mozilla.org/mozilla-central/rev/a1354a3e748e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.