Bug 16644 – REGRESSION (r28880-r28886): Global variable access

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Woody Gilk]

If you visit netvibes.com with WebKit, r28886 or later, it will cause 3 JS "TypeError: value undefined" errors.

This regression is not present in r28880 and earlier.

[David Kilzer (:ddkilzer)]

The bisect-builds script reports:
Works: r28880  Fails: r28886

Revision r28884 looks most suspicious.
http://trac.webkit.org/projects/webkit/changeset/28884

[David Kilzer (:ddkilzer)]

<rdar://problem/5665251>

[David Kilzer (:ddkilzer)]

The initial "TypeError: value undefined" error occurs on this statement:

HTMLElement.prototype.htmlElement=function(){};

The cause is that HTMLElement.prototype is undefined.  HTMLElement is defined as this when the statement is run:

function () 
{
}

[David Kilzer (:ddkilzer)]

Created attachment 18176 [details]
Reduction

This is a reduction of the original JavaScript from the web site.

With Safari 3.0.4 (523.12.2) with original WebKit on Mac OS X 10.4.11 (8S165), loading the test case, then loading this URL:

javascript:alert(HTMLElement)

Produces:

[object HTMLElementConstructor]

And thus the code in the if() statement never runs.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> With Safari 3.0.4 (523.12.2) with original WebKit on Mac OS X 10.4.11 (8S165),
> loading the test case, then loading this URL:
> 
> javascript:alert(HTMLElement)
> 
> Produces:
> 
> [object HTMLElementConstructor]
> 
> And thus the code in the if() statement never runs.

Ignore that.  The local debug build of WebKit r29032 does this as well.  The issue lies somewhere else.

[David Kilzer (:ddkilzer)]

Created attachment 18177 [details]
Better reduction

If the "var HTMLElement = function(){};" statement is removed (or commented out), the test passes.

However, having that statement inside the if() block causes the if() condition to return true, which I find to be completely bizarre.

[Oliver Hunt]

This is almost certainly due to r28884

As an educated guess the problem is like to be that in:
if (typeof HTMLElement == 'undefined') {
    var HTMLElement=function(){};
    document.write("FAIL");
} else {
    document.write("PASS");
}

the references to HTMLElement are replaced with fast indexed lookups into the symbol table, however when the symbol table is initialised it does not allow for the potential for these variables to be present on the global object, and just assumes that they are all undefined by default.I would guess that the fix would be to have the global symbol table be initalised from the values in the global object if there are any attempts to shadow any of the ghlobal objects properties.

[David Kilzer (:ddkilzer)]

The Netvibes site uses Mootools v1.11 as well.  See Bug 16605 and Bug 16679.

[David Kilzer (:ddkilzer)]

*** Bug 16702 has been marked as a duplicate of this bug. ***

[Matt Lilek]

This broke Lively Kernel as well - http://research.sun.com/projects/lively/index.xhtml

[Geoffrey Garen]

Great diagnosis, Oliver and Dave.

I think it would be best not to make an entry in the symbol table at all. In order to make an entry, you would need to know (a) that the existing property was DontDelete and (b) that it was a permanent property, and not any of the many global properties that come and go and return different values at different times.

Maybe we can figure out how to optimize this case down the line, though.

[Mark Rowe (bdash)]

*** Bug 16813 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

This is a very challenging bug. FF does what I suggested in Comment #11. IE 6 & 7, though, match current TOT, while Opera is somewhere in the middle. It's not clear what correct behavior would be, but I tend to prefer IE's behavior.

[Oliver Hunt]

(In reply to comment #13)
> This is a very challenging bug. FF does what I suggested in Comment #11. IE 6 &
> 7, though, match current TOT, while Opera is somewhere in the middle. It's not
> clear what correct behavior would be, but I tend to prefer IE's behavior.
> 

Remember to check the lively kernel when fixing this :D

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> This is a very challenging bug. FF does what I suggested in Comment #11. IE 6 &
> 7, though, match current TOT, while Opera is somewhere in the middle. It's not
> clear what correct behavior would be, but I tend to prefer IE's behavior.

Does Brendan Eich have a bugs.w.o account?  ;)  On a more serious note, perhaps he hangs out on one of the Mozilla IRC channels on irc.freenode.net?

[Geoffrey Garen]

Committed revision 29428.

[David Kilzer (:ddkilzer)]

*** Bug 16605 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

*** Bug 16679 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Ick.  FF's behavior seems poor here.  Ours is even worse (since we seem to silently fail instead of throwing an exception).  CCing myself with the intention of at least re-writing these tests to use the modern JS testing framework so that they run in FF and we can confirm that our behavior matches (which I'm not sure it does 100%).