Closed Bug 1254847 Opened 4 years ago Closed 4 years ago

Switch AutoEntryScript to always take ownership of error reporting

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Most already do.  I'll file specific bugs blocking this one on the remaining ones that don't do it.
Depends on: 1254848
Depends on: 1254855
Depends on: 1254857
Depends on: 1254859
Depends on: 1254860
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.
Flags: needinfo?(jst)
(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.
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.
Attachment #8728567 - Flags: review?(bobbyholley)
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.
Attachment #8728568 - Flags: review?(bobbyholley)
Attachment #8728567 - Flags: review?(bobbyholley) → review+
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.
Attachment #8728568 - Flags: review?(bobbyholley) → review+
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/
Attachment #8728579 - Flags: review?(bobbyholley) → review+
Blocks: 1255181
(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!
Flags: needinfo?(jst)
Blocks: 1255817
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.