522796 - [OOPP] Implement NPN_SetException

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

Implement NPN_SetException.

Comment 1

[Rolf Bjarne Kvinge]

This breaks a lot of tests for the Moonlight team. Will it ever be fixed?

Comment 2

[Benjamin Smedberg]

Are you testing that exceptions thrown from JS have particular strings in them? This is currently a very low priority and may never be fixed (I am considering proposing to plugin-futures that the API be made obsolete).

Comment 3

[Rolf Bjarne Kvinge]

No, we're testing that exceptions get thrown.

Comment 4

[Benjamin Smedberg]

Exceptions should always get thrown if you return 'false' from a NPRuntime function. If that's not working, please file a new bug. Note that calling NPN_SetException by itself should never throw an exception: it is only an indicator that the next exception thrown (by returning false) should have the error message specified.

Comment 5

[Rolf Bjarne Kvinge]

NPN_SetException throws an exception in ff4 with inproc plugins, ff3, google-chrome and opera, so I think you're wrong there.

However it looks like returning false will also throw exceptions as you say. I'll try that.

What I don't understand is why it's a bad thing to have a helpful exception message, since you want to obsolete that API.

Comment 6

[Rolf Bjarne Kvinge]

If I return false from NPRuntime functions chrome throws a generic error message, not the one set with NPN_SetException (and at the same time I found out we do have tests that check the actual error message).

So right now if I want ff4+oopp to throw errors, I can't get custom error messages in chrome.

Comment 7

[lekma]

Attachment: restore pre ipc behaviour

This patch is only a proposal for discussion.
pros: straightforward, cons: necessitate a change of API (NPN_SetException change its signature) and I don't know how to handle that (update NPAPI version??).

If the change of API is deemed acceptable, we can then improve the npruntime exception handling (throw real exception objects for example, etc.).

(In reply to comment #2)
> This is currently a very low priority and may never be fixed (I am
> considering proposing to plugin-futures that the API be made obsolete).
Do you intend to replace it with something else?

Comment 8

[Benjamin Smedberg]

Changing the API is a complete nonstarter. Any NPAPI changes need to be additive. You could add a NPN_SetExceptionObject, for example. But any API changes need to be proposed and discussed on the plugin-futures list before we deal with them in Firefox code.

Comment 9

[lekma]

Attachment: restore NPN_SetException

(In reply to comment #8)
> Changing the API is a complete nonstarter. Any NPAPI changes need to be
> additive. You could add a NPN_SetExceptionObject, for example. But any API
Ok, this new patch does not require an API change.

> changes need to be proposed and discussed on the plugin-futures list before
> we deal with them in Firefox code.
I'll try to come up with something to submit to plugin-futures regarding a better exception handling/error reporting API (I'm on holidays for the next few weeks though, so, if someone else wants to tackle it in the meantime).

Comment 10

[Benjamin Smedberg]

I fixed up a few nits in your patch and committed it r=me:

http://hg.mozilla.org/integration/mozilla-inbound/rev/80b410d518e6

It should end up in mozilla-central soon (merges from inbound happen daily, usually) for Firefox 8.

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/80b410d518e6