Most members of JSDebugHooks should be deleted

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla33
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Now that JSD is gone, all but one member of JSDebugHooks are unused. They should be removed, along with the code that supports them.
(Assignee)

Comment 1

4 years ago
Created attachment 8447718 [details] [diff] [review]
Remove unused elements of JSDebugHooks, and their supporting code.

Here's a patch that removes everything but JSDebugHooks::debuggerHandler and its closure pointer. I think I've chased down all the stuff that exists only to support those hooks.

A lovely diffstat:

 b/js/public/OldDebugAPI.h                                  |  109 --------
 b/js/src/builtin/Eval.cpp                                  |    2 
 b/js/src/frontend/BytecodeCompiler.cpp                     |   23 -
 b/js/src/frontend/BytecodeCompiler.h                       |    8 
 b/js/src/frontend/BytecodeEmitter.cpp                      |    2 
 b/js/src/jit/BaselineFrame.cpp                             |    5 
 b/js/src/jit/BaselineFrame.h                               |   18 -
 b/js/src/jit/BaselineJIT.cpp                               |   14 -
 b/js/src/jit/VMFunctions.cpp                               |    7 
 b/js/src/jsapi-tests/testDebugger.cpp                      |  171 -------------
 b/js/src/jsapi-tests/testSourcePolicy.cpp                  |   23 -
 b/js/src/jscntxt.cpp                                       |   25 -
 b/js/src/jsfun.cpp                                         |   21 -
 b/js/src/jsscript.cpp                                      |   29 --
 b/js/src/jsscript.h                                        |   13 
 b/js/src/shell/js.cpp                                      |   24 -
 b/js/src/vm/HelperThreads.cpp                              |   33 --
 b/js/src/vm/Interpreter.cpp                                |   10 
 b/js/src/vm/OldDebugAPI.cpp                                |  124 ---------
 b/js/src/vm/Stack-inl.h                                    |   32 --
 b/js/src/vm/Stack.cpp                                      |    1 
 b/js/src/vm/Stack.h                                        |   37 --
 js/src/jit-test/tests/jaeger/bug563000/test-throwhook-1.js |   19 -
 js/src/jit-test/tests/jaeger/bug563000/test-throwhook-2.js |   14 -
 24 files changed, 13 insertions(+), 751 deletions(-)
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8447718 - Flags: review?(sphink)
(Assignee)

Comment 3

4 years ago
Created attachment 8447721 [details] [diff] [review]
Remove unused elements of JSDebugHooks, and their supporting code.

Rats, that patch dropped calls to clearTraps. Here's another attempt.

Try: https://tbpl.mozilla.org/?tree=Try&rev=a26a22287052

Perhaps I should go to bed...
Attachment #8447718 - Attachment is obsolete: true
Attachment #8447718 - Flags: review?(sphink)
Attachment #8447721 - Flags: review?(sphink)
(Assignee)

Comment 4

4 years ago
Try push looks good.
Comment on attachment 8447721 [details] [diff] [review]
Remove unused elements of JSDebugHooks, and their supporting code.

Review of attachment 8447721 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, I somehow didn't notice this review request. And it was therapeutic reading all of the removed code.

r=me with the redundant stepModeEnabled stuff removed. Also, I'll give this an r+ if you remove the redundant stepModeEnabled stuff.

::: js/src/frontend/BytecodeCompiler.cpp
@@ -180,5 @@
> -frontend::MaybeCallSourceHandler(JSContext *cx, const ReadOnlyCompileOptions &options,
> -                                 SourceBufferHolder &srcBuf)
> -{
> -    JSSourceHandler listener = cx->runtime()->debugHooks.sourceHandler;
> -    void *listenerData = cx->runtime()->debugHooks.sourceHandlerData;

Oh, so debugHooks.sourceHandler is kinda like onBeforeCompileSource or whatever it's called? Huh.

::: js/src/jit/VMFunctions.cpp
@@ +891,3 @@
>  
> +    if (script->stepModeEnabled()) {
> +        if (script->stepModeEnabled())

Uh... you never can be too sure?

::: js/src/vm/Interpreter.cpp
@@ +1524,3 @@
>              RootedValue rval(cx);
>              JSTrapStatus status = JSTRAP_CONTINUE;
> +            if (script->stepModeEnabled())

That darn step mode. Keeps flapping back and forth.
Attachment #8447721 - Flags: review?(sphink) → review+
Duplicate of this bug: 908881
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a19c530f3d0
Flags: in-testsuite-
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/9a19c530f3d0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.