Closed
Bug 1070049
Opened 10 years ago
Closed 9 years ago
Remove nsJSUtils::ReportPendingException
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bholley, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 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 | ||
Comment 2•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Assignee | ||
Comment 3•10 years ago
|
||
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•10 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•10 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•10 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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Reporter | ||
Comment 8•10 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•10 years ago
|
||
Gah! Looks like I put the wrong bug# on the patch in bug 1098628.
Flags: needinfo?(terrence)
Assignee | ||
Comment 10•10 years ago
|
||
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•10 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+
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•