Closed Bug 575345 Opened 15 years ago Closed 15 years ago

Brower generates "Did you mean to access that lowercase property" message for declarations like "var FullScreen"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dmandelin, Assigned: dmandelin)

References

Details

Spun off from bug 574745 comment 5.
* Detailed Cause OK, I think I found out what's going on here. The error is actually generated by the statement 'var FullScreen = ...', specifically the DEFVAR. The DEFVAR does a lookup to see if the property is already defined, in which case there is an error. That lookup fails on its last chance, XPC_WN_ModsAllowed_Proto_Resolve. That function then calls DefinePropertyIfFound, which calls HandlePossibleNameCaseError. That function in turn calls XPCNativeSet::FindMember on the lower-case version of the name, 'fullScreen'. If FindMember returns true, then we generate the error message, but otherwise not. In TM (and m-c), FindMember returns false, but in fatvals, FindMember returns true. It appears that TM/m-c are the ones with the bug: in the final comparison function they use to find the property, they call XPCNativeInterface::FindMember, which is: // Tracemonkey/mozilla-central inline XPCNativeMember* XPCNativeInterface::FindMember(jsval name) const { const XPCNativeMember* member = mMembers; for(int i = (int) mMemberCount; i > 0; i--, member++) if(member->GetName() == name) return const_cast<XPCNativeMember*>(member); return nsnull; } Note that two string jsvals are being compared using '=='. This is wrong unless the strings are interned; otherwise two strings of the same value could compare unequal. fatvals does it right, comparing two jsids, which are required to be interned if strings. * Conclusions Now, it's fairly clear that merely declaring a name shouldn't produce an error like this. I'm not sure exactly what step in the chain is wrong. I guess we probably do want to call the resolve hook (after all, we have JSRESOLVE_DECLARING), so the error handling in XPConnect must be to blame. Should I just shut off calling HandlePossibleNameCaseError if we have JSRESOLVE_DECLARING?
Summary: fatvals: JS errors starting up browser. → Brower generates "Did you mean to access that lowercase property" message for declarations like "var FullScreen"
Specifically, fatval fixes a bug where a non-intern string was being stuffed into a jsval (which of course doesn't assert isAtomized) and then implicitly converted jsid.
(In reply to comment #1) > Should I just shut off calling HandlePossibleNameCaseError if we have > JSRESOLVE_DECLARING? Yes, or even in all cases, tbh. I think it has outlived its utility.
I'm seeing the warning show up a lot in automated test spew. I think this will annoy everyone so I'm taking comment 3 as a go-ahead to remove the check in fatvals.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.