Closed Bug 522796 Opened 11 years ago Closed 9 years ago

[OOPP] Implement NPN_SetException

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: benjamin, Assigned: malek)

References

Details

Attachments

(2 files)

Implement NPN_SetException.
No longer depends on: 522791
Depends on: 522791
Summary: [e10s] Implement NPN_SetException → [OOPP] Implement NPN_SetException
This breaks a lot of tests for the Moonlight team. Will it ever be fixed?
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).
No, we're testing that exceptions get thrown.
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.
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.
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.
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?
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.
(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).
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.
Assignee: bent.mozilla → malek
http://hg.mozilla.org/mozilla-central/rev/80b410d518e6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.