488285 - Missing assignment to undeclared variable warnings

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[[e-mail]]

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

Comment 1

[Igor Bukanov]

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.

Comment 2

[Igor Bukanov]

Attachment: v1

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.

Comment 3

[Igor Bukanov]

nominating for 1.9.1 as this bug is regression from a 1.9.1 blocker

Comment 4

[Igor Bukanov]

Attachment: v2

The new version uses comprehensible comments in nsDOMClassInfo.cpp.

Comment 5

[Brendan Eich [: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

Comment 6

[Igor Bukanov]

Attachment: v3

The new version of the patch fixes the nits.

Comment 7

[Brendan Eich [:brendan]]

Pretty sure this blocks; if not, highly wanted by web devs.

/be

Comment 8

[Brendan Eich [:brendan]]

And XUL devs, including Firefox FE devs.

/be

Comment 9

[[e-mail]]

It occurred to me to retry the test under xpcshell, which also fails. Is this patch supposed to fix that too?

Comment 10

[Igor Bukanov]

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

Comment 11

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/5d76df9bab84

Comment 12

[[e-mail]]

Should components and modules also be checked just in case they have the bug?

Comment 13

[Igor Bukanov]

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

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5d76df9bab84

Comment 15

[Brendan Eich [:brendan]]

Need a separate bug on the comment 12 issue?

/be

Comment 16

[Robert Sayre]

Merging failed on the 1.9.1 branch in nsDOMClassInfo.cpp

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/26e355e3f68e

Comment 18

[Aakash Desai [:aakashd]]

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