Closed Bug 488285 Opened 12 years ago Closed 12 years ago

Missing assignment to undeclared variable warnings

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: neil, Assigned: igor)

References

()

Details

(Keywords: regression, verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

Steps to reproduce problem:
1. Turn on strict JavaScript warnings
2. Open the Error Console
3. Evaluate x=3
Expected result: Warning: assignment to undeclared variable x
Assignee: general → igor
This is a regression from the bug 462734. Due to the changes there JSOP_BINDNAME no longer does any property lookup and delegates the reporting of the warning to JSOP_SETNAME under the assumption that this should be transparent to the resolve hooks. But this is not the case due to the following code from http://hg.mozilla.org/tracemonkey/file/50427c373190/dom/base/nsDOMClassInfo.cpp#l6525 :

  6530   JSStackFrame *fp = NULL;
  6531   if ((flags & JSRESOLVE_ASSIGNING) &&
  6532       !(flags & JSRESOLVE_WITH) &&
  6533       !(JS_FrameIterator(cx, &fp) && fp->regs && (JSOp)*fp->regs->pc == JSOP_BINDNAME) &&
  6534       win->IsInnerWindow()) {

...
  6560       // Define a fast expando, the key here is to use JS_PropertyStub
  6561       // as the getter/setter, which makes us stay out of XPConnect
  6562       // when using this property.

This change comes from bug 420858 and means that the code in nsWindowSH::NewResolve skips creating fast expando for JSOP_BINDNAME but not for JSOP_SETNAME. With the optimization from the bug 462734 that implies that the engine never sees that a property does not exists in the window objects.

Fix for that should be simple: instead of checking for JSOF_BINDNAME the resolve hook could report the strict warning when doing undeclared assigns.
Blocks: 462734
Attached patch v1 (obsolete) — Splinter Review
The patch changes nsWindowSH::NewResolve to warn when assigning to non-existing properties. For that the patch uses a helper function that is shared between NewResolve and js_SetPropertyHelper. 

The patch removes the unqualified argument from js_SetPropertyHelper. I added it in bug 462734 not to slow down code with extra checks that regs->pc points JSOP_SETNAME. 

But that just bloated all the callers to js_SetPropertyHelper. Those checks for JSOP_SETNAME are only necessary for the global object and checking that OBJ_GET_PARENT() is null is just as fast as checking if a boolean flag is false. And when the object is global, check's execution time must be completely eclipsed by the complexity of running global window resolve hook.
Attachment #372711 - Flags: review?(brendan)
nominating for 1.9.1 as this bug is regression from a 1.9.1 blocker
Flags: blocking1.9.1?
Attached patch v2 (obsolete) — Splinter Review
The new version uses comprehensible comments in nsDOMClassInfo.cpp.
Attachment #372711 - Attachment is obsolete: true
Attachment #372715 - Flags: review?(brendan)
Attachment #372711 - Flags: review?(brendan)
Comment on attachment 372715 [details] [diff] [review]
v2

>+JS_FRIEND_API(JSBool)
>+js_CheckUndeclaredVarAssign(JSContext *cx)

Should be named js_CheckUndeclaredVarAssignment.

>+{
>+    JSStackFrame *fp = js_GetTopStackFrame(cx);
>+    if (!JS_HAS_STRICT_OPTION(cx) ||
>+        !(fp = js_GetTopStackFrame(cx)) ||

Two js_GetTopStackFrame(cx) calls -- first must be a left-over. Ideally, fp's declaration won't need an initializer since no control flow can reach a use of fp without going through the set nested in the ! test in the || chain.

>+/*
>+ * Check if it is OK to assign an undeclared property of the global object

s/if/whether/

r=me with nits picked.

/be
Attachment #372715 - Flags: review?(brendan) → review+
Attached patch v3Splinter Review
The new version of the patch fixes the nits.
Attachment #372715 - Attachment is obsolete: true
Attachment #372750 - Flags: review+
Pretty sure this blocks; if not, highly wanted by web devs.

/be
And XUL devs, including Firefox FE devs.

/be
It occurred to me to retry the test under xpcshell, which also fails. Is this patch supposed to fix that too?
(In reply to comment #9)
> It occurred to me to retry the test under xpcshell, which also fails. Is this
> patch supposed to fix that too?

I am not sure - xpcshell global is not the windows object that the patch fixes.
landed to TM - http://hg.mozilla.org/tracemonkey/rev/5d76df9bab84
Whiteboard: fixed-in-tracemonkey
Should components and modules also be checked just in case they have the bug?
(In reply to comment #12)
> Should components and modules also be checked just in case they have the bug?

The regression fallout from the bug 462734 happens if resolve hook for JS class do something special for JSOP_BINDNAME bytecode or do something smart with the resolve flags. According to my queries to mxr.mozilla.org dom/base/nsDOMClassInfo.cpp was the only case of that.

So, if other components do not show warnings, it should be unrelated to this specific regression. At least this is my current thinking.
http://hg.mozilla.org/mozilla-central/rev/5d76df9bab84
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Need a separate bug on the comment 12 issue?

/be
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Merging failed on the 1.9.1 branch in nsDOMClassInfo.cpp
Flags: in-testsuite?
verified FIXED on builds: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090430 Minefield/3.6a1pre ID:20090430041822

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090430 Shiretoko/3.5b5pre ID:20090430044210
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.