Bug 95073 – WindowShell and global registers break IC

• Component: JavaScriptCore
• Status: UNCONFIRMED

[Yusuke Suzuki]

Scripts can add newly created JSGlobalObject register variables to JSGlobalObject.
But, JSGlobalObject structure is not reflect this change.
So if this variable hides property of global.[[Prototype]] or upper object, IC fails to load valid property.

For example,


// t001.js
Object.prototype.T = 3000;  // point for proto ICvar global = this;
function test(len, func) {
  for (var i = 0; i < len; ++i) {
    func(global.T);
  }
}
test(100, function() { });  // make test function compiled by baseline JIT (for proto IC)


load('t002.js');  // make global register in other script


print(T);  // of cource, function defined in t002.js
print(global.T);  // of cource, function defined in t002.js
test(1, print);  // Oops!



// t002.js
function T() { }  // this should hides Object.prototype.T


and


$ jsc t001.js

And, window proxy (WindowShell) implementation is simple proxy of JSGlobalObject.
But its structure doesn't reflect global variable changes. So this also breaks IC (see appended test html)

To fix this, when new global register is added, refresh structure of JSGlobalObject and invalidate IC.
And we should not cache proxy lookup result.

[Yusuke Suzuki]

Attached html sample
http://jsfiddle.net/PvDQw/

Later I'll attach patch for this issue.

[Yusuke Suzuki]

Created attachment 160696 [details]
Patch

attached patch v1

[Filip Pizlo]

Comment on attachment 160696 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowShell.cpp:99
> -    return thisObject->window()->methodTable()->getOwnPropertySlot(thisObject->window(), exec, propertyName, slot);
> +    const bool res = thisObject->window()->methodTable()->getOwnPropertySlot(thisObject->window(), exec, propertyName, slot);
> +    slot.forceUncacheable();
> +    return res;

Why do you need to forceUncacheable() in addition to the other change?

This looks like it could have severe performance consequences.

[Yusuke Suzuki]

(In reply to comment #3)
> (From update of attachment 160696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160696&action=review
> 
> > Source/WebCore/bindings/js/JSDOMWindowShell.cpp:99
> > -    return thisObject->window()->methodTable()->getOwnPropertySlot(thisObject->window(), exec, propertyName, slot);
> > +    const bool res = thisObject->window()->methodTable()->getOwnPropertySlot(thisObject->window(), exec, propertyName, slot);
> > +    slot.forceUncacheable();
> > +    return res;
> 
> Why do you need to forceUncacheable() in addition to the other change?
> 

Because WindowShell calls JSGlobalObject method simply, when JSGlobalObject structure is changed, WindowShell structure is not changed.

So, for example,

Object.prototype.T = 20;
window.T  // lookup

When lookup is executed(in JIT), this makes chain IC. Chain layout is following,

[WindowShell structure (not JSGlobalObject structure)]
[WindowPrototype structure]
[ObjectPrototype structure]

And we can define JSGlobalObject property like this.
window.T = 20;

When this code is executed, JSGlobalObject structure is changed properly, but window shell structure is not changed. So above chain IC is not invalidated.

Here is an example.
http://jsfiddle.net/V5tL6/
In this page, V8 & SpiderMonkey output 0 and 20 to console, but JSC outputs 0 twice. This is because chain IC isn't invalidated.

> This looks like it could have severe performance consequences.
I've checked this and I think this doesn't make severe perf regression.

Because chain IC first structure is always structure of WindowShell, self IC already fails now. (I saw this by adding breakpoint to JIT::tryCacheGetById. Is it right?)

And because resolve_global target is JSGlobalObject (not WindowShell), their IC still works fine.
This change only adds following pattern IC fails.

Object.getPrototypeOf(window).b = 20;
window.b;

// or
window.toString;

And so I think this performance regression is small.


But I have another idea; when JSGlobalObject structure is changed, notify it to all proxies to refresh their strucutre. What do you think about this?

[Geoffrey Garen]

Our typical defense against this type of bug is that the function doing the lookup and caching checks that the property owner (PropertySlot::slotBase()) is equal to the object on which it's doing the lookup, and refuses to cache if not. 

Do you know what goes wrong with that defense in this case?

[Yusuke Suzuki]

(In reply to comment #5)
> Our typical defense against this type of bug is that the function doing the lookup and caching checks that the property owner (PropertySlot::slotBase()) is equal to the object on which it's doing the lookup, and refuses to cache if not. 
> 
> Do you know what goes wrong with that defense in this case?

Yes. forceUncacheable is necessary for browser environment, not JSC shell.

In the shell, global object is JSGlobalObject, but in the browser, global object is JSDOMWindowShell object(this is proxy to JSGlobalObject)
WindowShell object has JSGlobalObject and lookup operations are passed to JSGlobalObject. But this makes a severe problem.
https://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSDOMWindowShell.h#L80

For example, we define own property to global and lookup.
window.a = 20;
window.a;

When lookup is executed, WindowShell lookups from JSGlobalObject.
https://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSDOMWindowShell.cpp#L94

As the result, when lookup is succceeded, slot.baseValue() is JSGlobalObject (not WindowShell).
But, `window` object(baseCell) is WindowShell. So currently JSC on Safari has already failed at self IC creation point.
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/jit/JITStubs.cpp#L932

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #5)
> > Our typical defense against this type of bug is that the function doing the lookup and caching checks that the property owner (PropertySlot::slotBase()) is equal to the object on which it's doing the lookup, and refuses to cache if not. 
> > 
> > Do you know what goes wrong with that defense in this case?
> 
> Yes. forceUncacheable is necessary for browser environment, not JSC shell.
> 
> In the shell, global object is JSGlobalObject, but in the browser, global object is JSDOMWindowShell object(this is proxy to JSGlobalObject)
> WindowShell object has JSGlobalObject and lookup operations are passed to JSGlobalObject. But this makes a severe problem.
> https://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSDOMWindowShell.h#L80
> 
> For example, we define own property to global and lookup.
> window.a = 20;
> window.a;
> 
> When lookup is executed, WindowShell lookups from JSGlobalObject.
> https://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSDOMWindowShell.cpp#L94
> 
> As the result, when lookup is succceeded, slot.baseValue() is JSGlobalObject (not WindowShell).
> But, `window` object(baseCell) is WindowShell. So currently JSC on Safari has already failed at self IC creation point.
> https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/jit/JITStubs.cpp#L932

I think this last paragraph explains why you don't need forceUncacheable().  Why do you need to force uncacheable when JSC already chooses not to perform caching since baseValue is not the same as slotBase?

[Yusuke Suzuki]

(In reply to comment #7)
> I think this last paragraph explains why you don't need forceUncacheable().  Why do you need to force uncacheable when JSC already chooses not to perform caching since baseValue is not the same as slotBase?

Yes. But this is self IC case, not proto or chain case.
self IC creation is guarded by this, but proto or chain IC isn't guarded.

So, chain IC example,

Object.prototype.a = 1;
function lookup() {
  window.a  // lookup
}
lookup();  // create chain IC #1
lookup();  // lookup from chain IC #2
window.a = 0;  // #3
lookup();  // lookup from invalid chain IC #4

when #1, lookup succeeded and slot.baseValue() == ObjectPrototype. So we can pass the chain IC condition and create chain IC on this point.
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/jit/JITStubs.cpp#L969
At this point, because `baseValue` is WindowShell, so prototype chain is like this.

[WindowShell structure (not JSGlobalObject structure)]
[WindowPrototype structure]
[ObjectPrototype structure]

Problem is that first structure stored in the StructureChain is WindowShell structure, not JSGlobalObject structure.
As the result, when #2, because `baseCell` is WindowShell, chain IC conditions are passed and lookup is executed by chain IC.

And when #3, we can add new property to JSGlobalObject. Because WindowShell passes put operation to JSGlobalObject, this new property is added to JSGlobalObject. At this time, JSGlobalObject structure transition(addPropertyTransition) occurs. But unfortunately, because this is JSGlobalObject structure transition, WindowShell structure isn't changed, this is problem.

As the result, when #4, because WindowShell structure isn't changed in spite of JSGlobalObject structure change, invalid chain IC conditions are passed unfortunately and chain IC lookup is executed ignoring JSGlobalObject property `a`.

`forceUncacheable` prevents IC creation on lookups through WindowShell object because WindowShell cannot reflect current object shape by its structure.

[Anders Carlsson]

Comment on attachment 160696 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.