Last Comment Bug 680562 - Remove CHECK_INTERRUPT_HANDLER
: Remove CHECK_INTERRUPT_HANDLER
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 650411 onStep
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-19 15:07 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-07-19 07:32 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm (16.25 KB, patch)
2012-07-08 03:08 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
rm (15.88 KB, patch)
2012-07-16 18:14 PDT, Luke Wagner [:luke]
jimb: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-08-19 15:07:37 PDT
Once JSRuntimes are single-threaded, we can replace CHECK_INTERRUPT_HANDLER with jimb's new mechanism for enabling interrupts.
Comment 1 Luke Wagner [:luke] 2011-08-19 15:18:31 PDT
Awesome, these are way gross.
Comment 2 Luke Wagner [:luke] 2012-05-03 10:50:20 PDT
I just noticed this is unblocked.
Comment 3 Luke Wagner [:luke] 2012-07-08 03:08:37 PDT
Created attachment 640040 [details] [diff] [review]
rm
Comment 4 Jim Blandy :jimb 2012-07-16 13:37:48 PDT
OMG (In reply to Jason Orendorff [:jorendorff] from comment #0)
> Once JSRuntimes are single-threaded, we can replace CHECK_INTERRUPT_HANDLER
> with jimb's new mechanism for enabling interrupts.

???? Oh, that. :(
Comment 5 Jim Blandy :jimb 2012-07-16 14:26:55 PDT
Shouldn't there be a change to JS_SetInterrupt, JS_ClearInterrupt, to actually use the InterpreterFrames chain, as JSScript::initScriptCounts and JSScript::ensureHasDebugScript do?
Comment 6 Jim Blandy :jimb 2012-07-16 14:29:24 PDT
Comment on attachment 640040 [details] [diff] [review]
rm

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

::: js/src/jsinterp.cpp
@@ -917,5 @@
>  template<typename T>
>  class GenericInterruptEnabler : public InterpreterFrames::InterruptEnablerBase {
>    public:
>      GenericInterruptEnabler(T *variable, T value) : variable(variable), value(value) { }
> -    void enableInterrupts() const { *variable = value; }

"There's no need to restate the type name in member function names." Thanks!

::: js/src/jsinterp.h
@@ +241,5 @@
>      ~InterpreterFrames();
>  
>      /* If this js::Interpret frame is running |script|, enable interrupts. */
>      inline void enableInterruptsIfRunning(JSScript *script);
> +    inline void enableInterruptsUnconditionally() { enabler.enable(); }

This has no callers, but it seems like it needs to be used by JS_SetInterrupt.
Comment 7 Luke Wagner [:luke] 2012-07-16 15:05:03 PDT
(In reply to Jim Blandy :jimb from comment #5)

You're right, but it's weird, because I remember writing that code; I guess I lost it somehow...

> This has no callers, but it seems like it needs to be used by JS_SetInterrupt.

Yeah, that is what I added for JS_SetInterrupt :P
Comment 8 Luke Wagner [:luke] 2012-07-16 18:14:03 PDT
Created attachment 642818 [details] [diff] [review]
rm

qref'd 'n everything

On a related note: has it every happened to you that you have some un-qref-d changes, then you push an empty patch for the try-server message, push to try, qpop, qdel the try patch, and then notice you've lost your changes?
Comment 9 Jim Blandy :jimb 2012-07-17 14:53:06 PDT
(In reply to Luke Wagner [:luke] from comment #8)
> On a related note: has it every happened to you that you have some un-qref-d
> changes, then you push an empty patch for the try-server message, push to
> try, qpop, qdel the try patch, and then notice you've lost your changes?

Well, if you have uncommitted changes and qnew (to add the try chooser text), then that qnew will gather up the uncommitted changes into the new patch. And then you pop and qdel that. Possible?
Comment 10 Jim Blandy :jimb 2012-07-17 14:55:50 PDT
VC tools should never lose your work; qdel should totally support "undo". But I think it's right for qnew to gather up uncommitted changes.
Comment 11 Luke Wagner [:luke] 2012-07-17 15:05:09 PDT
(In reply to Jim Blandy :jimb from comment #9)
Yes, that's exactly what happens.  I've had to retrieve the patch from try server before :)
Comment 12 Jim Blandy :jimb 2012-07-17 15:09:58 PDT
Comment on attachment 642818 [details] [diff] [review]
rm

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

Okay --- that makes more sense!
Comment 14 Ed Morley [:emorley] 2012-07-19 07:32:56 PDT
https://hg.mozilla.org/mozilla-central/rev/8e406f71fadb

Note You need to log in before you can comment on or make changes to this bug.