Closed Bug 1070049 Opened 5 years ago Closed 4 years ago

Remove nsJSUtils::ReportPendingException

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bholley, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

This is a subset of the work for bug 981187, and is needed more urgently for bug 1067574. We basically need to implement an opt-in mechanism for reporting exceptions with AutoJSAPI (as opposed to with the error reporter), and then use that for all of the call paths that land us in ReportPendingException.

I've been hacking on this, and am part of the way there.
Depends on: 1070131
Oh I see. LoadFrameScriptInternal calls nsJSUtils::ReportPendingException directly (I thought it went through nsJSUtils::EvaluateString). That significantly reduces the scope of the work needed to unblock bug 1067574. I'll update the bug hierarchy, possibly once I'm on a better connection.
Depends on: 1070842
No longer blocks: 1067574
Depends on: 70842
No longer depends on: 1070131, 1070842
Depends on: 1070842
Attached patch rpe_xuldocument-v0.diff (obsolete) — Splinter Review
Remove the ReportPendingException instance in XULDocument::ExecuteScript and add a test to verify that the error path continues to fire correctly.
Assignee: bobbyholley → terrence
Status: NEW → ASSIGNED
Attachment #8521750 - Flags: review?(bobbyholley)
Comment on attachment 8521750 [details] [diff] [review]
rpe_xuldocument-v0.diff

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

Looks great!

I assume you confirmed somehow that your test does in fact trigger this codepath, right?

::: dom/xul/XULDocument.cpp
@@ +3593,5 @@
>      JSAutoCompartment ac(cx, global);
>  
>      // The script is in the compilation scope. Clone it into the target scope
>      // and execute it.
> +    JS::CloneAndExecuteScript(cx, global, scriptObject);

Add a comment saying that we don't need to check the return value because aes will handle exceptions.
Attachment #8521750 - Flags: review?(bobbyholley) → review+
(If you want to land this immediately, I'd request that you file a dependent bug, because I'm a hater about [leave open] landings). :P
(In reply to Bobby Holley (:bholley) from comment #4)
> Comment on attachment 8521750 [details] [diff] [review]
> rpe_xuldocument-v0.diff
> 
> Review of attachment 8521750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great!
> 
> I assume you confirmed somehow that your test does in fact trigger this
> codepath, right?

Yes, I injected a MOZ_CRASH if !JS::CloneAndExecuteScript and did indeed hit that crash.
 
> ::: dom/xul/XULDocument.cpp
> @@ +3593,5 @@
> >      JSAutoCompartment ac(cx, global);
> >  
> >      // The script is in the compilation scope. Clone it into the target scope
> >      // and execute it.
> > +    JS::CloneAndExecuteScript(cx, global, scriptObject);
> 
> Add a comment saying that we don't need to check the return value because
> aes will handle exceptions.

Good idea!

(In reply to Bobby Holley (:bholley) from comment #5)
> (If you want to land this immediately, I'd request that you file a dependent
> bug, because I'm a hater about [leave open] landings). :P

Some of the other instances look like they're going to be substantially more work than this, so I'll start filing dependent bugs.
Depends on: 1098025
Depends on: 1098074
Depends on: 1098628
Depends on: 1099278
Depends on: 1099287
Depends on: 1099425
https://hg.mozilla.org/mozilla-central/rev/659549fa7af0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
I don't think this is fixed - terrence, did you land something with the wrong bug number?
Status: RESOLVED → REOPENED
Flags: needinfo?(terrence)
Resolution: FIXED → ---
Gah! Looks like I put the wrong bug# on the patch in bug 1098628.
Flags: needinfo?(terrence)
EvaluateString is the last user, so once bug 1099425 lands, this can follow on its heels.
Attachment #8521750 - Attachment is obsolete: true
Attachment #8527068 - Flags: review?(bobbyholley)
Comment on attachment 8527068 [details] [diff] [review]
rm_nsjsutils_reportpendingexception-v0.diff

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

\o/
Attachment #8527068 - Flags: review?(bobbyholley) → review+
I fixed this in bug 1174486.
Depends on: 1174486
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.