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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

It is allowed to remove debuggee globals from inside the onStep handler.
Currently this causes the stepModeCount to get into an inconsistent state.

Comment 1

[Shu-yu Guo [:shu]]

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

Test for this is 'resumption-08.js' over in the tests part of bug 716647.

Comment 2

[Shu-yu Guo [:shu]]

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

Comment 3

[Shu-yu Guo [:shu]]

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

Comment 4

[Shu-yu Guo [:shu]]

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

Had a typo, also testing git-bz

Comment 5

[Jim Blandy :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?

Comment 6

[Shu-yu Guo [:shu]]

(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

[Jim Blandy :jimb]

As long as the OSR patches make resumption-08 fail, that's fine. I'd forgotten the details.

Comment 8

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1354a3e748e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1354a3e748e