Last Comment Bug 522796 - [OOPP] Implement NPN_SetException
: [OOPP] Implement NPN_SetException
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla8
Assigned To: lekma
:
Mentors:
Depends on: 522791
Blocks: OOPP
  Show dependency treegraph
 
Reported: 2009-10-16 14:13 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2011-07-27 03:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
restore pre ipc behaviour (8.15 KB, patch)
2011-07-22 13:08 PDT, lekma
no flags Details | Diff | Splinter Review
restore NPN_SetException (3.66 KB, patch)
2011-07-25 01:38 PDT, lekma
no flags Details | Diff | Splinter Review

Description Benjamin Smedberg [:bsmedberg] 2009-10-16 14:13:25 PDT
Implement NPN_SetException.
Comment 1 Rolf Bjarne Kvinge 2011-05-04 06:53:15 PDT
This breaks a lot of tests for the Moonlight team. Will it ever be fixed?
Comment 2 Benjamin Smedberg [:bsmedberg] 2011-05-04 07:27:26 PDT
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 2011-05-04 16:45:27 PDT
No, we're testing that exceptions get thrown.
Comment 4 Benjamin Smedberg [:bsmedberg] 2011-05-05 06:06:47 PDT
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 2011-05-05 06:49:49 PDT
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 2011-05-06 15:50:02 PDT
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 2011-07-22 13:08:26 PDT
Created attachment 547784 [details] [diff] [review]
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 [:bsmedberg] 2011-07-23 06:21:19 PDT
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 2011-07-25 01:38:14 PDT
Created attachment 548121 [details] [diff] [review]
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 [:bsmedberg] 2011-07-26 11:14:19 PDT
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] 2011-07-27 03:25:57 PDT
http://hg.mozilla.org/mozilla-central/rev/80b410d518e6

Note You need to log in before you can comment on or make changes to this bug.