Closed Bug 1798068 Opened 2 years ago Closed 2 years ago

static analysis failure, with NS_DECL_NSIXPCSCRIPTABLE causing "error: The fully qualified types are preferred over the shorthand typedefs for JS::Handle/JS::Rooted types outside SpiderMonkey."

Categories

(Core :: XPConnect, defect)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
108 Branch
Tracking Status
firefox108 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files)

I just tried building with static analysis enabled for the first time in a while. My local build uses distributed sccache which does preprocessor-expansion separately from compilation, for what it's worth.

And I'm hitting this build error:

 0:13.82 In file included from Unified_cpp_storage1.cpp:29:
 0:13.82 In file included from /scratch/work/builds/mozilla-central/mozilla/storage/mozStorageStatement.cpp:19:
 0:13.82 /scratch/work/builds/mozilla-central/mozilla/storage/mozStorageStatementJSHelper.h:23:391: error: The fully qualified types are preferred over the shorthand typedefs for JS::Handle/JS::Rooted types outside SpiderMonkey.
 0:13.82   virtual nsresult GetClassName(nsACString& aClassName) override; virtual uint32_t GetScriptableFlags(void) override; virtual const JSClass * GetJSClass(void) override; virtual nsresult PreCreate(nsISupports *nativeObj, JSContext * cx, JSObject * globalObj, JSObject * * parentObj) override; virtual nsresult NewEnumerate(nsIXPConnectWrappedNative *wrapper, JSContext * cx, JSObject * obj, JS::MutableHandleIdVector properties, bool enumerableOnly, bool *_retval) override; virtual nsresult Resolve(nsIXPConnectWrappedNative *wrapper, JSContext * cx, JSObject * obj, jsid id, bool *resolvedp, bool *_retval) override; virtual nsresult Finalize(nsIXPConnectWrappedNative *wrapper, JS::GCContext * gcx, JSObject * obj) override; virtual nsresult Call(nsIXPConnectWrappedNative *wrapper, JSContext * cx, JSObject * obj, const JS::CallArgs & args, bool *_retval) override; virtual nsresult Construct(nsIXPConnectWrappedNative *wrapper, JSContext * cx, JSObject * obj, const JS::CallArgs & args, bool *_retval) override; virtual nsresult HasInstance(nsIXPConnectWrappedNative *wrapper, JSContext * cx, JSObject * obj, JS::Handle<JS::Value> val, bool *bp, bool *_retval) override;
 0:13.82                                                                                                                                                                                                                                                                                                                                                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~
 0:13.82                                                                                                                                                                                                                                                                                                                                                                                                       JS::MutableHandleVector<JS::PropertyKey>

The compile error points to the last line here, the NS_DECL_NSIXPCSCRIPTABLE macro:
https://searchfox.org/mozilla-central/rev/fe5c9c39a879b07d5b629257f63d825c3c8cd0ed/storage/mozStorageStatementJSHelper.h#20-23

class StatementJSHelper : public nsIXPCScriptable {
 public:
  NS_DECL_ISUPPORTS
  NS_DECL_NSIXPCSCRIPTABLE

And in particular it seems to be complaining about an arg for the GetClassName API [EDIT: actually it's the NewEnumerate API, I skimmed the single-line API declaration too quickly], which is declared (in the expanded macro) as JS::MutableHandleIdVector properties and static analysis warning is saying it should instead be declared with type JS::MutableHandleVector<JS::PropertyKey> I guess.

Kagami, looks like this was a linter that you added -- would you mind taking a look? (Presumably we need to either update an allow-list somewhere, or fix the definition of NS_DECL_NSIXPCSCRIPTABLE which would mean a tweak in XPIDL or something like that.)

Flags: needinfo?(krosylight)

Here's a manually-pruned section of the error message that's pointing to the warned-about arg:

virtual nsresult NewEnumerate([...], JSObject * obj, JS::MutableHandleIdVector properties, 
                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~
                                                     JS::MutableHandleVector<JS::PropertyKey>

That seems to correspond to this chunk of IDL:
https://searchfox.org/mozilla-central/rev/fe5c9c39a879b07d5b629257f63d825c3c8cd0ed/js/xpconnect/idl/nsIXPCScriptable.idl#75-77

boolean newEnumerate(in nsIXPConnectWrappedNative wrapper,
                    in JSContextPtr cx, in JSObjectPtr obj,
                    in JSMutableHandleIdVector properties,

...with the type alias defined further up in that file at line 31:
https://searchfox.org/mozilla-central/rev/fe5c9c39a879b07d5b629257f63d825c3c8cd0ed/js/xpconnect/idl/nsIXPCScriptable.idl#31

      native JSMutableHandleIdVector(JS::MutableHandleIdVector);

Maybe line 31 just needs to change to reference JS::MutableHandleVector<JS::PropertyKey>...?

Yup, that seems to do it.

Assignee: nobody → dholbert
Flags: needinfo?(krosylight)
Component: Source Code Analysis → XPConnect
Product: Developer Infrastructure → Core

This is a non-functional change (just whitespace cleanup).

This patch doesn't change behavior; it's just expanding an abbreviated typename.

Without this patch, my local static-analysis build fails with the following,
fore.g. mozStorageStatementJSHelper.h (which has a class that implements
nsIXPCScriptable and is outside of spidermonkey):

"error: The fully qualified types are preferred over the shorthand typedefs for
JS::Handle/JS::Rooted types outside SpiderMonkey."

This patch's type-expansion seems to appease this static analysis rule.

Before this patch, the type here was JS::MutableHandleIdVector which is a
typedef for MutableHandle<StackGCVector<JS::PropertyKey>> as defined here:
https://searchfox.org/mozilla-central/rev/fe5c9c39a879b07d5b629257f63d825c3c8cd0ed/js/public/TypeDecls.h#98

After this patch, the type here is JS::MutableHandleVector<JS::PropertyKey>
(which I took from the static analysis error message's suggestion). That
expands to the same full-qualified type, since MutableHandleVector<T> is an
alias for MutableHandle<StackGCVector<T>> as defined here:
https://searchfox.org/mozilla-central/rev/fe5c9c39a879b07d5b629257f63d825c3c8cd0ed/js/public/TypeDecls.h#128

Depends on D160689

Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/af0f176408d1 part 1: Fix indentation on some mis-indented lines of nsIXPCScriptable.idl. r=xpcom-reviewers,mccr8 https://hg.mozilla.org/integration/autoland/rev/e10250770801 part 2: Expand JS::MutableHandleIdVector to more verbose typename in nsIXPCScriptable.idl, to satisfy static-analysis rule. r=xpcom-reviewers,mccr8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 108 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: