GNOME Bugzilla – Bug 668517
browser-plugin: Add a new "onshellrestart" API
Last modified: 2012-01-31 00:49:54 UTC
This was suggested in Owen's review comments when the browser-plugin first landed ( bug #658070 ). I don't remember the comment, so I think I just skimmed over it. Nevertheless, here's a patch that adds it.
Created attachment 205910 [details] [review] browser-plugin: Add a new "onshellrestart" API This function is something the client sets and is called whenever the Shell's DBus name is acquired.
Review of attachment 205910 [details] [review]: Mostly fine, see one problem with memory management in an error case. ::: browser-plugin/browser-plugin.c @@ +316,3 @@ + + funcs.invokeDefault (obj->instance, obj->restart_listener, + NULL, 0, &result); As far as I can find, there's no guarantee that result is initialized on a false return and for Mozilla, it will in fact be uninitialized in the false return case. (Webkit seems to always initialize). So, I think both this and the on_shell_signal usage of invokeDefault need to set an initial value of result. (As in get_string_property()) @@ +338,3 @@ + G_BUS_NAME_WATCHER_FLAGS_NONE, + on_shell_appeared, + NULL, I don't have a strong opinion whether this should be called when the bus name is lost or not - I think it's OK as you have it here - we don't support switching the session between shell and fallback on the fly, so the no-shell case should be at most temporary. @@ +848,3 @@ + } + else if (NPVARIANT_IS_NULL (*value)) + return TRUE; Hmm, it's a little ugly to have onrestart = 1; both throw an exception and null out the old listener, really should do one or the other, but practically speaking it's 100% fine.
*** Bug 666443 has been marked as a duplicate of this bug. ***
(In reply to comment #2) > Review of attachment 205910 [details] [review]: > As far as I can find, there's no guarantee that result is initialized on a > false return and for Mozilla, it will in fact be uninitialized in the false > return case. (Webkit seems to always initialize). So, I think both this and the > on_shell_signal usage of invokeDefault need to set an initial value of result. > (As in get_string_property()) > > @@ +338,3 @@ > + G_BUS_NAME_WATCHER_FLAGS_NONE, > + on_shell_appeared, > + NULL, > > I don't have a strong opinion whether this should be called when the bus name > is lost or not - I think it's OK as you have it here - we don't support > switching the session between shell and fallback on the fly, so the no-shell > case should be at most temporary. Right. And even then, the website's use of "onshellrestart" would be to do something like refresh the current extensions list -- not very helpful when we don't have a shell session.
Created attachment 206064 [details] [review] browser-plugin: Add a new "onshellrestart" API This function is something the client sets and is called whenever the Shell's DBus name is acquired.
Created attachment 206065 [details] [review] browser-plugin: Add a new "onshellrestart" API This function is something the client sets and is called whenever the Shell's DBus name is acquired.
Created attachment 206066 [details] [review] browser-plugin: Fix callback for "onchange" In the case that calling the listener fails, "result" may be uninitialized. Sending NPAPI uninitialized memory is never a good idea.
Created attachment 206067 [details] [review] browser-plugin: Refactor plugin_object_set_property, and fix a bug If the user did "obj.onchanged = 1;" or passed another sort of invalid type, then we would clear the old listener as well as throw an exception.
Review of attachment 206066 [details] [review]: Looks good
Review of attachment 206065 [details] [review]: Looks good
Review of attachment 206067 [details] [review]: ::: browser-plugin/browser-plugin.c @@ +810,3 @@ } +static inline bool the compiler will inline a static like this if it make sense so skip 'inline' - this function doesn't even look like a good inlining candidate - it's not too short and it is rarely called. @@ +815,3 @@ +{ + if (!NPVARIANT_IS_OBJECT (*value) || !NPVARIANT_IS_NULL (*value)) + return FALSE; Not what you meant to write
Created attachment 206145 [details] [review] browser-plugin: Refactor plugin_object_set_property, and fix a bug If the user did "obj.onchanged = 1;" or passed another sort of invalid type, then we would clear the old listener as well as throw an exception.
Review of attachment 206145 [details] [review]: Looks good
Attachment 206065 [details] pushed as 3bcdba6 - browser-plugin: Add a new "onshellrestart" API Attachment 206066 [details] pushed as 1556344 - browser-plugin: Fix callback for "onchange" Attachment 206145 [details] pushed as 2699198 - browser-plugin: Refactor plugin_object_set_property, and fix a bug
this now makes the plugin crash in chromium :P (reading NULL in plugin_object_set_callback()) I don't have a resolved trace at hand but this 0x10 sure looks weird: #0 plugin_object_set_callback (listener=0x10, value=<optimized out>) at browser-plugin.c:819 No locals. #1 0xb65ca2d9 in NPObjectStub::OnSetProperty(NPIdentifier_Param const&, NPVariant_Param const&, IPC::Message*) () No symbol table info available. ...
Created attachment 206484 [details] [review] browser-plugin: Fix the browser plugin commit 26991988cb76a9bd2f43700a98212099bdb023fb broke the browser plugin by trying to reference a uninitialized pointer and making the NPAPI retain a NULL object. This is embarrasingly my fault. It seems that while testing the browser plugin, I missed a crucial step: *copy to ~/.mozilla/plugins/*.
Comment on attachment 206484 [details] [review] browser-plugin: Fix the browser plugin Attachment 206484 [details] pushed as 2c9e6bb - browser-plugin: Fix the browser plugin Pushed after IRC approval from Owen.