1476557 - [Static Analysis] DEAD_STORE errors in dom/plugins/*

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Robert Bartlensky [:rbartlensky]]

dom/plugins/base/nsPluginHost.cpp:495: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  493.   nsPluginHost::ActuallyReloadPlugins()
  494.   {
  495. >   nsresult rv = NS_OK;
  496.
  497.     // shutdown plugins and kill the list if there are no running plugins

dom/plugins/base/nsNPAPIPlugin.cpp:1060: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  1058.     }
  1059.     obj = js::GetGlobalForObjectCrossCompartment(obj);
  1060. >   nsresult rv = NS_OK;
  1061.     {
  1062.       nsJSUtils::ExecutionContext exec(cx, obj);

dom/plugins/base/nsPluginHost.cpp:1426: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  1424.     nsPluginTag* pluginTag = FindNativePluginForType(aMimeType, true);
  1425.     if (pluginTag) {
  1426. >     rv = NS_OK;
  1427.       PLUGIN_LOG(PLUGIN_LOG_BASIC,
  1428.       ("nsPluginHost::GetPlugin Begin mime=%s, plugin=%s\n",

dom/plugins/base/nsPluginHost.cpp:3281: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  3279.     if (owner) {
  3280.       nsCOMPtr<nsIURI> baseURI = owner->GetBaseURI();
  3281. >     rv = NS_MakeAbsoluteURI(absUrl, aURL, baseURI);
  3282.     }
  3283.

dom/plugins/base/nsPluginHost.cpp:3392: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  3390.                                     nsIChannel *aGenericChannel)
  3391.   {
  3392. >   nsresult rv = NS_OK;
  3393.
  3394.     nsCOMPtr<nsIHttpChannel> aChannel = do_QueryInterface(aGenericChannel);

Comment 1

[Robert Bartlensky [:rbartlensky]]

Attachment: Bug 1476557: Fix DEAD_STORE errors reported by infer in dom/plugins/*.

Review commit: https://reviewboard.mozilla.org/r/257788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257788/

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8992941 [details]
Bug 1476557: Fix DEAD_STORE errors reported by infer in dom/plugins/*.

https://reviewboard.mozilla.org/r/257788/#review264800

Thanks.

::: commit-message-8dab9:1
(Diff revision 1)
> +Bug 1476557: Fix DEAD_STORE errors in dom/plugins/*. r?froydnj

Maybe "Fix DEAD_STORE errors reported by infer in..."?

::: dom/plugins/base/nsPluginHost.cpp:3277
(Diff revision 1)
>    // get the base URI for the plugin to create an absolute url
>    // in case aURL is relative
>    RefPtr<nsPluginInstanceOwner> owner = aInstance->GetOwner();
>    if (owner) {
>      nsCOMPtr<nsIURI> baseURI = owner->GetBaseURI();
> -    rv = NS_MakeAbsoluteURI(absUrl, aURL, baseURI);
> +    Unused << NS_MakeAbsoluteURI(absUrl, aURL, baseURI);

Does infer complain if you don't provide `Unused <<`?  Trying to handle every `nsresult` return value is a noble goal, but I'm not sure the return on investment is very good...

::: dom/plugins/base/nsPluginHost.cpp:3388
(Diff revision 1)
>  nsresult
>  nsPluginHost::AddHeadersToChannel(const char *aHeadersData,
>                                    uint32_t aHeadersDataLen,
>                                    nsIChannel *aGenericChannel)
>  {
> -  nsresult rv = NS_OK;
> +  nsresult rv;

It looks like all the sets of `rv` in this function are basically useless.  Why don't we move the declaration of this down to the `SetRequestHeader` call in the loop, and fixup everything else?

Comment 3

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8992941 [details]
Bug 1476557: Fix DEAD_STORE errors reported by infer in dom/plugins/*.

https://reviewboard.mozilla.org/r/257788/#review264800

> Does infer complain if you don't provide `Unused <<`?  Trying to handle every `nsresult` return value is a noble goal, but I'm not sure the return on investment is very good...

DEAD_STORE errors mean that if a variable is given a value and it is not used, then why bother giving the variable the value in the first place? In this case (someone suggested in another bug) that using `Unused` might be a better choice than dropping the return value. But it might be worth asserting the value instead, which is faster I *think*.

Comment 4

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8992941 [details]
Bug 1476557: Fix DEAD_STORE errors reported by infer in dom/plugins/*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257788/diff/1-2/

Comment 5

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8992941 [details]
Bug 1476557: Fix DEAD_STORE errors reported by infer in dom/plugins/*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257788/diff/2-3/

Comment 6

[Robert Bartlensky [:rbartlensky]]

See comment #2 and #3.