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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

Spun off from bug 574745 comment 5.

Comment 1

[David Mandelin [:dmandelin]]

* 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?

Comment 2

[Luke Wagner [:luke]]

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.

Comment 3

[Mike Shaver (:shaver emeritus)]

(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.

Comment 4

[Luke Wagner [:luke]]

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.