451571 - Delete SetExceptionWasThrown

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jason Orendorff [:jorendorff]]

There's only one call to GetExceptionWasThrown, and it can use JS_IsExceptionPending instead.

Deleting this will reduce the number of calls to GetCurrentNativeCallContext.  See bug 407216 comment 95 paragraph 2 and subsequent discussion.

Comment 1

[Jason Orendorff [:jorendorff]]

I am a little puzzled about the apparently common idiom in COM-style methods of setting a pending JS exception, then returning NS_OK.  Examples:

nsJSContext::EvaluateStringWithValue
mozJSComponentLoader::ReportOnCaller (and its callers)
nsJSCID::CreateInstance when the security manager vetoes
mozJSSubScriptLoader::LoadSubScript

It seems weird not to tell the caller that the call failed.  Why does it work this way?

Comment 2

[Jason Orendorff [:jorendorff]]

Re comment 1, here's one explanation:
http://hg.mozilla.org/mozilla-central/index.cgi/file/73967cc9e4ee/js/src/xpconnect/public/nsAXPCNativeCallContext.h#l67

but it seems like CallMethod should just see that an exception is already pending and just propagate that.

Comment 3

[Jason Orendorff [:jorendorff]]

Contrary to the URL comment cited in comment 2, there are also places where a C++ function sets an exception and then returns a failure nsresult:

nsScriptSecurityManager::CheckPropertyAccessImpl
nsJSScriptTimeoutHandler::Init
xpcJSWeakReference::Init

Maybe there should be an NS_ERROR_JS_EXCEPTION_THROWN code for this case.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v2

(Sorry for the noise.  The only difference between v1 and v2 is that v1 had a few lines of nonsense in xpcprivate.h.)

Comment 6

[David Bradley]

Almost done with the review, should have it done this evening or tomorrow 8/24

Comment 7

[David Bradley]

I need to run a couple of more scenario's in my head. I'm just concerned that we might end up capturing more than what we were before, though I'm thinking that's probably not a bad thing. The code looks fine.

Comment 8

[David Bradley]

Comment on attachment 334937 [details] [diff] [review]
v2

r=dbradley

Changes look fine. My only concern was that we might pick up some JS exceptions that we weren't picking up before in the XPCWrappedNative::CallMethod. But I don't think that's necessarily a bad thing and might be considered a flaw in the original logic.

The patch is a bit stale, there were a couple of hunks that wouldn't apply, but was easy enough to work around.

Comment 9

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/mozilla-central/rev/b4d9de8ad106