Closed Bug 1252268 Opened 4 years ago Closed 4 years ago

If the slow script dialog aborts a script, that was invoked by an interval timer, kill the timer.


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox48 --- fixed


(Reporter: khuey, Assigned: khuey)



(Whiteboard: btpp-active)


(2 files, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
This fixes bug 1251475 on my Linux system.
Attachment #8724974 - Flags: review?(bzbarsky)
Comment on attachment 8724974 [details] [diff] [review]

>+++ b/dom/bindings/CallbackObject.cpp
>+    if (mErrorResult.ErrorCodeIs(NS_ERROR_JS_CALL_FAILED)) {
>+      // Translate the exception to something more useful.
>+      if (needToDealWithException) {
>+        mErrorResult.Throw(NS_ERROR_DOM_JS_EXCEPTION);

This is going to fail an assert in ErrorResult::AssignErrorCode.  You'd probably want to throw NS_ERROR_UNEXPECTED to match the old behavior in this case.

But also, the other option is to put this branching inside the codegenned code and just directly ThrowUncatchableException() in there if !JS_HasPendingException(cx).  This has the benefit of not needing NS_ERROR_JS_CALL_FAILED and slightly simpler code in ~CallSetup (no changes needed to it at all) but the drawback of slightly bloatier codegenned code.  I think in practice I'd prefer this option because there's less magic involved.

r=me with that.  And good catch on all those things that claim to throw and never do!
Attachment #8724974 - Flags: review?(bzbarsky) → review+
Whiteboard: btpp-active
Attached patch PatchSplinter Review
Attachment #8724974 - Attachment is obsolete: true
Attachment #8725443 - Flags: review?(bzbarsky)
Comment on attachment 8725443 [details] [diff] [review]

>+ErrorResult::NoteJSContextException(JSContext* aCx) {

Curly on next line, please.

Attachment #8725443 - Flags: review?(bzbarsky) → review+
Now with a test, and one important fix.  If NoteJSContextException sets the ErrorResult to NS_ERROR_DOM_EXCEPTION_ON_JSCONTEXT, and ~CallSetup doesn't rethrow the exception, we have to change the result code to something else.  For now I just used NS_ERROR_UNEXPECTED to preserve the existing behavior.

testPromiseWithThrowingChromePromiseInit caught that.
Attachment #8726462 - Flags: review?(bzbarsky)
Comment on attachment 8726462 [details] [diff] [review]
Fixup some fallout

We probably want the fixup to NS_ERROR_UNEXPECTED for promise then callbacks too.

>+    'TestFunctions.cpp',

Should that be debug-only, perhaps, and the header file export?  See the TestInterface* bits that are a bit below this part.

Attachment #8726462 - Flags: review?(bzbarsky) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.