Bug 183087 - Safari not handling undefined global variables with same name as element Id correctly.
Summary: Safari not handling undefined global variables with same name as element Id c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 11
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://www.ecma-international.org/ecm...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-23 10:20 PST by tylerdah77
Modified: 2018-03-08 23:12 PST (History)
10 users (show)

See Also:


Attachments
WIP patch (1.24 KB, patch)
2018-03-07 14:25 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.38 MB, application/zip)
2018-03-07 15:47 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.82 MB, application/zip)
2018-03-07 15:58 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.19 MB, application/zip)
2018-03-07 16:08 PST, EWS Watchlist
no flags Details
Patch (5.93 KB, patch)
2018-03-07 16:28 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.99 KB, patch)
2018-03-07 16:30 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tylerdah77 2018-02-23 10:20:05 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-02-26 19:14:33 PST
<rdar://problem/37927596>
Comment 2 Chris Dumez 2018-02-26 20:55:23 PST
Confirmed:
http://jsbin.com/pigefuriso/edit?html,output
Comment 3 Chris Dumez 2018-02-26 21:02:49 PST
Replacing:
var someElement;
with
var someElement = 1;

fixes the issues.

So we only ignore the local property if its value is undefined.
Comment 4 tylerdah77 2018-02-26 21:10:13 PST
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.
Comment 5 Chris Dumez 2018-02-26 21:11:54 PST
(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.
Comment 6 tylerdah77 2018-02-26 21:14:39 PST
Sorry, I thought you posted a 'solution' haha. Disregard.
Comment 7 Keith Miller 2018-02-26 22:41:41 PST
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.
Comment 8 Chris Dumez 2018-02-27 10:17:58 PST
(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.
Comment 9 Chris Dumez 2018-03-07 12:23:40 PST
(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.
Comment 10 Chris Dumez 2018-03-07 12:53:18 PST
(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().
Comment 11 Chris Dumez 2018-03-07 14:25:40 PST
Created attachment 335221 [details]
WIP patch
Comment 12 EWS Watchlist 2018-03-07 15:47:29 PST
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
Comment 13 EWS Watchlist 2018-03-07 15:47:31 PST
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
Comment 14 EWS Watchlist 2018-03-07 15:58:02 PST
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
Comment 15 EWS Watchlist 2018-03-07 15:58:04 PST
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
Comment 16 EWS Watchlist 2018-03-07 16:08:42 PST
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
Comment 17 EWS Watchlist 2018-03-07 16:08:43 PST
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
Comment 18 Chris Dumez 2018-03-07 16:28:36 PST
Created attachment 335240 [details]
Patch
Comment 19 Chris Dumez 2018-03-07 16:30:12 PST
Created attachment 335241 [details]
Patch
Comment 20 Chris Dumez 2018-03-08 12:17:07 PST
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
Comment 21 WebKit Commit Bot 2018-03-08 23:12:19 PST
Comment on attachment 335241 [details]
Patch

Clearing flags on attachment: 335241

Committed r229451: <https://trac.webkit.org/changeset/229451>
Comment 22 WebKit Commit Bot 2018-03-08 23:12:21 PST
All reviewed patches have been landed.  Closing bug.