Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX release] [Fixes #15492] WorkAround WebKit/JSC JIT WorkARound #15502

Merged
merged 1 commit into from
Jul 14, 2017

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Jul 14, 2017

Affected version: Version 10.0.3 (12602.4.8)
Unaffected versions: Safari 11.0, WebKit 12604.1.29

Which suggests the issue has been addressed upstream.


The following commit 986710f#diff-7e13eecefe753df1d82ce67b32bc4366R566 remove an implicit toBoolean conversion in WeakMap_peekParentMeta which apparently trips up Webkit…

Simply reversing the expression addressed the issue.

failed: if (meta === null || meta !== undefined) { … }
passed: if (meta !== undefined || meta === null) { … }

We also, confirmed that reverting the one line in question to meta === null || !meta) { fixed Safari.

This PR removes the need for the an unused feature which avoids the expression entirely.

@krisselden / @stefanpenner

@stefanpenner stefanpenner force-pushed the jit-fun branch 2 times, most recently from 581dd95 to a676ef2 Compare July 14, 2017 01:22
@stefanpenner
Copy link
Member Author

@stefanpenner stefanpenner force-pushed the jit-fun branch 2 times, most recently from c759eaa to 89b311b Compare July 14, 2017 02:08
@stefanpenner stefanpenner requested a review from krisselden July 14, 2017 02:30
@@ -518,7 +518,7 @@ if (HAS_NATIVE_WEAKMAP) {
// stop if we find a `null` value, since
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment seems incorrect

@@ -638,7 +638,7 @@ const ArrayMixin = Mixin.create(Enumerable, {
function EachProxy(content) {
this._content = content;
this._keys = undefined;
this.__ember_meta__ = null;
this.__ember_meta__ = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this worth keeping?

Copy link
Member Author

@stefanpenner stefanpenner Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can strip it once we drop support for old browsers entirely. For now, keeping the shape stable sorta important?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not then call meta(this) instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid making any significant changes for bugfix we intend to backport without proper measurement.

But I can gladly make the change

@@ -509,7 +509,7 @@ if (HAS_NATIVE_WEAKMAP) {
peekMeta = function WeakMap_peekParentMeta(obj) {
let pointer = obj;
let meta;
while (pointer !== undefined && pointer !== null) {
while (pointer !== undefined && null !== pointer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this for? this seems unrelated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting...

@stefanpenner stefanpenner force-pushed the jit-fun branch 2 times, most recently from f5c2548 to 83ea077 Compare July 14, 2017 02:47
Affected version: 10.1.1 (12603.2.4)
Unaffected version: Safari 11.0, WebKit 12604.1.29

Which suggests the issue has been addressed upstream.

---

The following commit 986710f#diff-7e13eecefe753df1d82ce67b32bc4366R566 remove an implicit toBoolean conversion in WeakMap_peekParentMeta which apparently trips up Webkit…

Simply reversing the expression addressed the issue.

failed: `if (meta === null || meta !== undefined) { … }`
passed: `if (meta !== undefined || meta === null) { … }`

We also, confirmed that reverting the one line in question to `meta === null || !meta) {` fixed Safari.

This PR removes the need for the an unused feature which avoids the expression entirely.

@krisselden / @stefanpenner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants