Closed Bug 575345 Opened 11 years ago Closed 11 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: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.