383524 - JS_GetProperty and other JSAPI functions should not generate an "undefined property" warning

Status: RESOLVED DUPLICATE of bug 1619177

(Core :: JavaScript Engine, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Consider the following stack:

2. js3250.dll!js_ReportValueErrorFlags(JSContext * cx=0x0442d880, unsigned int flags=5, const unsigned int errorNumber=162, int spindex=0, long v=47705748, JSString * fallback=0x00000000, const char * arg1=0x00000000, const char * arg2=0x00000000)

1. js3250.dll!js_GetProperty(JSContext * cx=0x0442d880, JSObject * obj=0x065b9e80, long id=47829376, long * vp=0x0012ea88)

0. xul.dll!nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject(XPCCallContext & ccx={...}, JSObject * jsobj=0x065b9e80, const nsID & aIID={...})

In this case my JS object doesn't have a QI function, so js_GetProperty logs a strict warning. Rather than printing "QI doesn't exist", however, it warns that the last property on the stack doesn't exist.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

So the problem is that we can't really use the decompile value generator when we reach js_GetProperty from a C API entrance. If we do, the DVG will find the currently executing script and make decisions based on that, which is obviously incorrect.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Perhaps we should add an internal API, GetCurrentFrame, or something that returns the current JSStackFrame if no JS_* native has been called between the latest js_Interpret and the current function. This function can return some sort of cookie (or hacked-up frame) for the various PC-inspecting functions, fixing bugs like this and bug 389034.

Comment 3

[:Irving Reid (No longer working on Firefox)]

This caused headaches for Thunderbird in Bug 646307 and, based on a quick Bugzilla search, is bothering lots of other people too.

For the specific case of a JS object missing QueryInterface(), could this check be done on the way in, so that the JS->XPCOM boundary would warn when you pass in an object without QI?

Comment 4

[[e-mail]]

People pass JS objects to XPConnect without QI all the time, so that's a non-starter.

Presumably bug 646307 was caused by XPConnect deciding to try to QI the object to nsIClassInfo for some reason. (nsIMsgHdrSink would have been OK, since XPConnect would have cached that on the way in.) I guess double-wrapping JS objects isn't something we do very commonly except with components where you need to provide QI anyway, but even so, QI not existing shouldn't be a warning.

Comment 5

[:Irving Reid (No longer working on Firefox)]

OK, if it's normal for JS objects to go through XPConnect without a QueryInterface implementation, we shouldn't warn on it. Should we move that discussion to one of the "I'm getting a spurious warning" bugs and leave this one to represent only the stack-frame-unravelling issue, or should we make this the master and point the related bugs here?

Comment 6

[Jason Orendorff [:jorendorff]]

The warning code looks like this now:

            RootedValue val(cx, IdToValue(id));
            if (!js_ReportValueErrorFlags(cx, flags, JSMSG_UNDEFINED_PROP,
                                          JSDVG_IGNORE_STACK, val, js::NullPtr(),
                                          nullptr, nullptr))
            {
                return false;
            }

Note that we pass id to js_ReportValuErrorFlags instead of trying to reconstruct it by looking at interpreter state. (JSDVG_IGNORE_STACK is also good.)

However, this warning really shouldn't happen if you're calling in from the API. Leaving this open, renaming.

Comment 7

[Tom S [:evilpie]]

Bug 1619177 completely removed that warning.