Bug 16644 - REGRESSION (r28880-r28886): Global variable access
Summary: REGRESSION (r28880-r28886): Global variable access
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Geoffrey Garen
URL: http://netvibes.com/
Keywords: InRadar, NeedsReduction, Regression
: 16605 16679 16702 16813 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-12-28 11:42 PST by Woody Gilk
Modified: 2008-04-03 12:48 PDT (History)
12 users (show)

See Also:


Attachments
Reduction (298 bytes, text/html)
2007-12-29 21:49 PST, David Kilzer (:ddkilzer)
no flags Details
Better reduction (161 bytes, text/html)
2007-12-29 22:00 PST, David Kilzer (:ddkilzer)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Woody Gilk 2007-12-28 11:42:16 PST
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.
Comment 1 David Kilzer (:ddkilzer) 2007-12-29 19:30:19 PST
The bisect-builds script reports:
Works: r28880  Fails: r28886

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

Comment 2 David Kilzer (:ddkilzer) 2007-12-29 19:31:15 PST
<rdar://problem/5665251>
Comment 3 David Kilzer (:ddkilzer) 2007-12-29 21:32:58 PST
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 () 
{
}
Comment 4 David Kilzer (:ddkilzer) 2007-12-29 21:49:52 PST
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.
Comment 5 David Kilzer (:ddkilzer) 2007-12-29 21:51:51 PST
(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.
Comment 6 David Kilzer (:ddkilzer) 2007-12-29 22:00:08 PST
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.
Comment 7 Oliver Hunt 2007-12-29 22:29:50 PST
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.
Comment 8 David Kilzer (:ddkilzer) 2007-12-30 09:43:47 PST
The Netvibes site uses Mootools v1.11 as well.  See Bug 16605 and Bug 16679.

Comment 9 David Kilzer (:ddkilzer) 2008-01-02 07:37:13 PST
*** Bug 16702 has been marked as a duplicate of this bug. ***
Comment 10 Matt Lilek 2008-01-10 17:38:17 PST
This broke Lively Kernel as well - http://research.sun.com/projects/lively/index.xhtml
Comment 11 Geoffrey Garen 2008-01-11 09:58:22 PST
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.
Comment 12 Mark Rowe (bdash) 2008-01-11 10:30:55 PST
*** Bug 16813 has been marked as a duplicate of this bug. ***
Comment 13 Geoffrey Garen 2008-01-11 13:16:31 PST
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.
Comment 14 Oliver Hunt 2008-01-11 13:25:45 PST
(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

Comment 15 David Kilzer (:ddkilzer) 2008-01-11 13:54:46 PST
(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?

Comment 16 Geoffrey Garen 2008-01-11 23:20:45 PST
Committed revision 29428.
Comment 17 David Kilzer (:ddkilzer) 2008-01-12 10:52:16 PST
*** Bug 16605 has been marked as a duplicate of this bug. ***
Comment 18 David Kilzer (:ddkilzer) 2008-01-12 10:56:55 PST
*** Bug 16679 has been marked as a duplicate of this bug. ***
Comment 19 Eric Seidel (no email) 2008-04-03 12:48:52 PDT
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%).