Remove nsJSUtils::ReportPendingException

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bholley, Assigned: terrence)

Tracking

unspecified
mozilla36
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Depends on: 1070131
(Reporter)

Comment 1

4 years ago
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.
(Reporter)

Updated

4 years ago
Depends on: 1070842
(Reporter)

Updated

4 years ago
No longer blocks: 1067574
Depends on: 70842
No longer depends on: 1070131, 1070842
(Reporter)

Updated

4 years ago
Depends on: 1070842
(Assignee)

Comment 3

4 years ago
Created attachment 8521750 [details] [diff] [review]
rpe_xuldocument-v0.diff

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)
(Reporter)

Comment 4

4 years ago
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+
(Reporter)

Comment 5

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

Comment 6

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

Updated

4 years ago
Depends on: 1098025
(Assignee)

Updated

4 years ago
Depends on: 1098074
(Assignee)

Updated

4 years ago
Depends on: 1098628
(Assignee)

Updated

4 years ago
Depends on: 1099278
(Assignee)

Updated

4 years ago
Depends on: 1099287
(Assignee)

Updated

4 years ago
Depends on: 1099425
https://hg.mozilla.org/mozilla-central/rev/659549fa7af0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Reporter)

Comment 8

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

Comment 9

4 years ago
Gah! Looks like I put the wrong bug# on the patch in bug 1098628.
Flags: needinfo?(terrence)
(Assignee)

Comment 10

4 years ago
Created attachment 8527068 [details] [diff] [review]
rm_nsjsutils_reportpendingexception-v0.diff

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)
(Reporter)

Comment 11

4 years ago
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
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.