The default bug view has changed. See this FAQ.

Remove CHECK_INTERRUPT_HANDLER

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: luke)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

rm
15.88 KB, patch
jimb
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Once JSRuntimes are single-threaded, we can replace CHECK_INTERRUPT_HANDLER with jimb's new mechanism for enabling interrupts.
(Reporter)

Updated

6 years ago
Depends on: 673125
(Reporter)

Updated

6 years ago
Depends on: 650411
(Assignee)

Comment 1

6 years ago
Awesome, these are way gross.

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: Other Branch → Trunk
(Assignee)

Comment 2

5 years ago
I just noticed this is unblocked.
(Assignee)

Comment 3

5 years ago
Created attachment 640040 [details] [diff] [review]
rm
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #640040 - Flags: review?(jimb)

Comment 4

5 years ago
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

5 years ago
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

5 years ago
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.
(Assignee)

Comment 7

5 years ago
(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
(Assignee)

Comment 8

5 years ago
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?
Attachment #640040 - Attachment is obsolete: true
Attachment #640040 - Flags: review?(jimb)
Attachment #642818 - Flags: review?(jimb)

Comment 9

5 years ago
(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

5 years ago
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.
(Assignee)

Comment 11

5 years ago
(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

5 years ago
Comment on attachment 642818 [details] [diff] [review]
rm

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

Okay --- that makes more sense!
Attachment #642818 - Flags: review?(jimb) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e406f71fadb
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/8e406f71fadb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.