545980 - orange in wake of bug 542002 (test failure log includes substring "test_empty_tags.js | 2 == undefined")

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

SOLO_FILE="test_empty_tags.js" make -C ./toolkit/components/places/tests/ check-one

to reproduce. Trying to reduce now.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: fairly minimal test

Could reduce more to hide its origins but this is good enough.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

A placeholder definition gains JSOP_CALLEE as its op when we leave the function with the used named, where the uses are inside that function's body (possibly in nested functions).

The function must be a named lambda, otherwise we could not be sure of its name still being bound (bindings including function definitions are mutable in general, const excepted).

But a lambda that is immediately applied loses its eagerly-set funarg flag. JSCompiler::markFunArgs knew enough to set this flag due to uses of a heretofore-non-funarg in escaping nested functions, but it did not deal with JSOP_CALLEE placeholder definitions.

/be

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 427557 [details] [diff] [review]
proposed fix

It is a bit funny that we're calling lexdep->isFunArg() there before we know if it's even relevant or not:

>                 if (!lexdep->isFreeVar() &&
>                     !lexdep->isFunArg() &&
>-                    lexdep->kind() == JSDefinition::FUNCTION) {
>+                    (lexdep->kind() == JSDefinition::FUNCTION ||
>+                     PN_OP(lexdep) == JSOP_CALLEE)) {

It would be clearer, I think, to put the isFunArg call after we know that lexdep->kind() == FUNCTION.

Otherwise looks fine.

Comment 4

[Brendan Eich [:brendan]]

I thought about that but the callee case is uncommon, and it's cheaper to test early and set the funarg flag in two dns, the placeholder and the named lambda node, for the hard callee case.

If we did reorder tests as you propose, we could have to test the other node (the named lambda node) instead of lexdep. So that would be a code size hit too, and slower for the hard case on second..Nth visits in this algorithm.

/be

Comment 5

[Brendan Eich [:brendan]]

BTW, this was a bug latent in the upvar2 patch, exposed by bug 542002's patch.

/be

Comment 6

[Brendan Eich [:brendan]]

We talked on IRC about synthesizing a proper JSDefinition::FUNCTION definition for the named lambda, but it's not that kind of definition -- it is not a hoisted binding in a variable object whose value is a function object. It's a lambda expr with a name in scope in its body. So the placeholder eagerly created to define the uses of that name serves here, but it is an atom-named definition node, not a funbox-referencing definition node.

Could certainly abstract and beautify, at some cost. I tried a more strung-out fix, but this spot-fix was simpler all around, modulo the sunk complexity costs that fixing this bug should not try to un-sink.

/be

Comment 7

[Brendan Eich [:brendan]]

The obvious question is why not make a definition node of some kind, instead of letting a placeholder be automatically created, for a named function expression?

The answer is mainly that doing so requires an extra compile-time scope structure of some kind. The function name goes in a lexical (declarative) environment that lies behind the static scope of the function's body. So

  print(function foo() { var foo = 42; return foo; }());

prints 42, not a decompilation of the named function expression.

Often people give function expressions names for decompilation and debugging purposes, without ever calling the name or otherwise using it in the body. This is done for methods created by initializing or assigning from a named lambda.

So secondarily, I wanted to avoid runtime costs, but primarily I wanted to avoid code complexity in the compiler, in setting up a two-tiered static scope chain. The placeholder machinery is there for good reason (use before possibly outer definition, out of order), and it happened to help here: if you end up in LeaveFunction with a use of the lambda's name, you know no var foo = 42; or any such inner-function shadowing binding existed, so you can impute JSOP_CALLEE.

Comments welcome as usual.

/be

Comment 8

[Brendan Eich [:brendan]]

The static scope structure would be two-tiered only in the silly sense that the outer tier would have the lambda's name and no other bindings. Maybe this is easy to do with a single-tier-plus-one-off-JSTreeContext-member.

I will file a followup bug and cite it in the patch, but get the patch landed to clear the orange.

/be

Comment 9

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/9b9745f43c78

I went without the FIXME and followup bug, as I'd been down this road before, trying to bind named function expression names eagerly. It's strictly more code statically and cycle-wise. Gonna stand pat here.

/be

Comment 11

[Nicholas Nethercote [inactive]]

"WINNT 5.2 tracemonkey debug test xpcshell" now has this:

s: win32-slave49TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\tracemonkey-win32-debug-unittest-xpcshell\build\xpcshell\tests\test_places\unit\test_adaptive.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | e:\builds\moz2_slave\tracemonkey-win32-debug-unittest-xpcshell\build\xpcshell\tests\test_places\unit\test_adaptive.js | application crashed (minidump found)
Thread 0 (crashed)

No idea if it's related, but I thought it worth mentioning.

Comment 12

[Brendan Eich [:brendan]]

I'm gonna say not me:

Crash reason:  EXCEPTION_ACCESS_VIOLATION
Crash address: 0x0

Thread 0 (crashed)
 0  xul.dll!TouchBadMemory [nsDebugImpl.cpp:9b9745f43c78 : 368 + 0x5]
    eip = 0x61b00448   esp = 0x0012c6b8   ebp = 0x0012c6b8   ebx = 0x0169f9d0
    esi = 0x00000005   edi = 0x02b98a18   eax = 0x00000001   ecx = 0x0012c6e0
    edx = 0x00000029   efl = 0x00010206
    Found by: given as instruction pointer in context
 1  xul.dll!Abort [nsDebugImpl.cpp:9b9745f43c78 : 376 + 0x4]
    eip = 0x61b00418   esp = 0x0012c6c0   ebp = 0x0012c6c0
    Found by: call frame info
 2  xul.dll!NS_DebugBreak_P [nsDebugImpl.cpp:9b9745f43c78 : 356 + 0xb]
    eip = 0x61b00156   esp = 0x0012c6c8   ebp = 0x0012cad8
    Found by: call frame info
 3  xul.dll!nsNavHistory::InternalAddVisit(__int64,__int64,__int64,__int64,int,__int64 *) [nsNavHistory.cpp:9b9745f43c78 : 1947 + 0x20]
    eip = 0x61852968   esp = 0x0012cae0   ebp = 0x0012cba8
    Found by: call frame info
 4  xul.dll!nsNavHistory::AddVisit(nsIURI *,__int64,nsIURI *,int,int,__int64,__int64 *) [nsNavHistory.cpp:9b9745f43c78 : 2769 + 0x32]
    eip = 0x618559ee   esp = 0x0012cbb0   ebp = 0x0012cd34
    Found by: call frame info

Line 1947 is

  NS_ASSERTION(hasResult, "hasResult is false but the call succeeded?");

Sayre, sdwilsh may know whether this needs separate filing. It doesn't go here.

/be

Comment 13

[Shawn Wilsher :sdwilsh]

It's been a while since I've touched that code, but mak is much more familiar with it.  If he doesn't chime in, I can look tomorrow.

Comment 14

[Bob Clary [:bc] (inactive)]

fixed manifest http://hg.mozilla.org/tracemonkey/rev/3970759ca2c1

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9b9745f43c78