1254847 - Switch AutoEntryScript to always take ownership of error reporting

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Most already do.  I'll file specific bugs blocking this one on the remaining ones that don't do it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

With the above patches, we're left with the two hard cases:

1)  nsXPCWrappedJSClass::DelegatedQueryInterface.  Should this just take ownership of error reporting?  Seems like it should be fine: if a QI method throws anything other than NS_NOINTERFACE, we should just report it and propagate NS_NOINTERFACE out to callers.  And if it throws NS_NOINTERFACE then we already clear the pending exception in nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject, then turn the null return value into a C++ NS_NOINTERFACE return.

2)  The AutoEntryScript uses in nsJSNPRuntime.cpp: doInvoke, NP_GetProperty, NP_SetProperty.  These are all followed by AutoJSExceptionReporter which in fact calls TakeOwnershipOfErrorReporting.  But before we do that AutoJSExceptionReporter thing, we might ThrowJSException a string.  Which itself might either set a pending exception (which is then reported by ... whom?) if PeekException() returns non-null or just call JS_ReportError.  Obviously the weird case is if we ThrowJSException and PeekException() returns non-null.  Johnny, do you happen to know what the goal of that code is?  I think you have blame for it.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> With the above patches, we're left with the two hard cases:
> 
> 1)  nsXPCWrappedJSClass::DelegatedQueryInterface.  Should this just take
> ownership of error reporting?  Seems like it should be fine: if a QI method
> throws anything other than NS_NOINTERFACE, we should just report it and
> propagate NS_NOINTERFACE out to callers.  And if it throws NS_NOINTERFACE
> then we already clear the pending exception in
> nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject, then turn the null return
> value into a C++ NS_NOINTERFACE return.

Seems very reasonable to me.

> 2)  The AutoEntryScript uses in nsJSNPRuntime.cpp: doInvoke, NP_GetProperty,
> NP_SetProperty.  These are all followed by AutoJSExceptionReporter which in
> fact calls TakeOwnershipOfErrorReporting.  But before we do that
> AutoJSExceptionReporter thing, we might ThrowJSException a string.  Which
> itself might either set a pending exception (which is then reported by ...
> whom?) if PeekException() returns non-null or just call JS_ReportError. 
> Obviously the weird case is if we ThrowJSException and PeekException()
> returns non-null.  Johnny, do you happen to know what the goal of that code
> is?  I think you have blame for it.

Given how much the invariants of the error reporting setup have morphed over the years, I'd be surprised if there was much rhyme or reason to read out of those tea leaves.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Take ownership of error reporting on the AutoEntryScript in nsXPCWrappedJSClass::DelegatedQueryInterface

If a QI method throws anything other than NS_NOINTERFACE, we should just report
it and propagate NS_NOINTERFACE out to callers.  And if it throws
NS_NOINTERFACE, then we already clear the pending exception in
nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject, then turn the null return
value into a C++ NS_NOINTERFACE return.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change nsJSNPRuntime to always use AutoEntryScript and always take ownership of error reporting

The main change is that we move AutoJSExceptionReporter up to before the first
place where we might throw an exception, so we report those exceptions before
returning.  The change to use AutoEntryScript consistently is because all of
these callsites can run JS in practice, and it seems reasonable to allow them
to.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8728568 [details] [diff] [review]
part 2.  Change nsJSNPRuntime to always use AutoEntryScript and always take ownership of error reporting

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +131,5 @@
>  
>  } // namespace
>  
> +// Helper class that suppresses any JS exceptions that were thrown while
> +// the plugin executed JS, if the nsJSObjWrapper has a destroy pending.

I might add a comment that this is the product of a long evolution of different things, and that the destroypending stuff (and thus the existence of this class) may or may not still be necessary.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make AutoEntryScript always take ownership of error reporting

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8728579 [details] [diff] [review]
part 3.  Make AutoEntryScript always take ownership of error reporting

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

\o/

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dbc870688e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fce4deb07b12
https://hg.mozilla.org/integration/mozilla-inbound/rev/781240d96e26

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5dbc870688e
https://hg.mozilla.org/mozilla-central/rev/fce4deb07b12
https://hg.mozilla.org/mozilla-central/rev/781240d96e26

Comment 10

[Johnny Stenback (:jst)]

(In reply to Boris Zbarsky [:bz] from comment #1)
[...]
> returns non-null.  Johnny, do you happen to know what the goal of that code
> is?  I think you have blame for it.

I wouldn't be surprised if I have blame for this, but I think Bobby is exactly right in that this has morphed through enough changes to make it close to impossible to reason from the original intent as to why the code before you cleaned this up now did exactly what it did. So thanks for digging through all this!