Bug 183087 – Safari not handling undefined global variables with same name as element Id correctly.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

I believe this to be a bug. 

If I have '<div id="someElement"></div>' in my DOM, a 'global' variable called someElement is created, this is OK, as it is fairly standard behavior.

Now, if I declare a global variable: 'var someElement;' This should now overshadow any global made with the element Id to make naming collisions with variables and elements not an issue. In Chrome, IE, Edge, FireFox, and Opera, my 'someElement' variable is 'undefined', as we would expect.

In Safari... 'someElement' is actually equal to '<div id="someElement"></div>' instead of 'undefined'. This is not expected behavior and seems to be a bug when every other browser correctly handles this.

[Radar WebKit Bug Importer]

<rdar://problem/37927596>

[Chris Dumez]

Confirmed:
http://jsbin.com/pigefuriso/edit?html,output

[Chris Dumez]

Replacing:
var someElement;
with
var someElement = 1;

fixes the issues.

So we only ignore the local property if its value is undefined.

That breaks standard browser behavior.

I initialize that variable to a widget later, so I count on it being undefined to only execute certain code if it is defined.

The easiest solution is to change the element name to something unique, but it shouldn't have to be.

[Chris Dumez]

(In reply to tylerdah77 from comment #4)
> That breaks standard browser behavior.
> 
> I initialize that variable to a widget later, so I count on it being
> undefined to only execute certain code if it is defined.
> 
> The easiest solution is to change the element name to something unique, but
> it shouldn't have to be.

Oh, this was not meant as a workaround for you. Just giving us a hint as to where the bug could be.

Sorry, I thought you posted a 'solution' haha. Disregard.

[Keith Miller]

Without looking closer I would guess this is because of the way that JSGlobalObject does getOwnPropertySlot and addGlobalVariable. If you define a variable it won't clear the existing global property, which I assume is the '<div id="someElement"></div>'. And when you do a getOwnPropertySlot on the global object it will see the old global property instead of shadowing. This is just a hunch, I can debug it more thoroughly later.

[Chris Dumez]

(In reply to Keith Miller from comment #7)
> Without looking closer I would guess this is because of the way that
> JSGlobalObject does getOwnPropertySlot and addGlobalVariable. If you define
> a variable it won't clear the existing global property, which I assume is
> the '<div id="someElement"></div>'. And when you do a getOwnPropertySlot on
> the global object it will see the old global property instead of shadowing.
> This is just a hunch, I can debug it more thoroughly later.

This would be great, thanks Keith.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to Keith Miller from comment #7)
> > Without looking closer I would guess this is because of the way that
> > JSGlobalObject does getOwnPropertySlot and addGlobalVariable. If you define
> > a variable it won't clear the existing global property, which I assume is
> > the '<div id="someElement"></div>'. And when you do a getOwnPropertySlot on
> > the global object it will see the old global property instead of shadowing.
> > This is just a hunch, I can debug it more thoroughly later.
> 
> This would be great, thanks Keith.

FYI, I do not see JSGlobalObject::addGlobalVar(const Identifier&) called. Therefore, JSGlobalObject::getOwnPropertySlot() is not able to find the variable in the symbol table.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > (In reply to Keith Miller from comment #7)
> > > Without looking closer I would guess this is because of the way that
> > > JSGlobalObject does getOwnPropertySlot and addGlobalVariable. If you define
> > > a variable it won't clear the existing global property, which I assume is
> > > the '<div id="someElement"></div>'. And when you do a getOwnPropertySlot on
> > > the global object it will see the old global property instead of shadowing.
> > > This is just a hunch, I can debug it more thoroughly later.
> > 
> > This would be great, thanks Keith.
> 
> FYI, I do not see JSGlobalObject::addGlobalVar(const Identifier&) called.
> Therefore, JSGlobalObject::getOwnPropertySlot() is not able to find the
> variable in the symbol table.

I see now. JSGlobalObject::addVar() gets called but it is implemented like so:
void addVar(ExecState* exec, const Identifier& propertyName)
    {
        if (!hasProperty(exec, propertyName))
            addGlobalVar(propertyName);
    }

hasProperty() returns true here (because of named property) so we do not call addGlobalVar().

[Chris Dumez]

Created attachment 335221 [details]
WIP patch

[EWS Watchlist]

Comment on attachment 335221 [details]
WIP patch

Attachment 335221 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6845587

New failing tests:
fast/forms/listbox-visible-size.html
js/dom/var-declarations-shadowing.html

[EWS Watchlist]

Created attachment 335230 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335221 [details]
WIP patch

Attachment 335221 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6845655

New failing tests:
fast/forms/listbox-visible-size.html
js/dom/var-declarations-shadowing.html

[EWS Watchlist]

Created attachment 335233 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335221 [details]
WIP patch

Attachment 335221 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6845568

New failing tests:
js/dom/var-declarations-shadowing.html
fast/forms/listbox-visible-size.html

[EWS Watchlist]

Created attachment 335237 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 335240 [details]
Patch

[Chris Dumez]

Created attachment 335241 [details]
Patch

[Chris Dumez]

Comment on attachment 335241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335241&action=review

> Source/JavaScriptCore/runtime/JSGlobalObject.h:514
> +        if (!hasOwnProperty(exec, propertyName))

See spec at http://www.ecma-international.org/ecma-262/6.0/#sec-candeclareglobalvar

[WebKit Commit Bot]

Comment on attachment 335241 [details]
Patch

Clearing flags on attachment: 335241

Committed r229451: <https://trac.webkit.org/changeset/229451>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.