Fix Safari GC issue by removing mapValues() in favor of Object.keys().reduce()

[paularmstrong]

Problem

See the gist: Safari GC Issue: Normalizr + Lodash (credit to @alunny)

Summary: normalizr + lodash in Safari appears to encounter a problem sometimes in which Safari's garbage collector interrupts the entity mapping, causing the entities object to be empty.

Solution

This is sort of a half-way-there solution, because the ultimate goal would be to remove all production dependencies. However, isEqual/deep-equality is not an easy problem to solve and lodash's implementation is the best available.

• Removes lodash dependency
• Uses lodash.isequal package for isEqual
• Adds an isObject util method
• Refactors use of lodash/mapValues

[gaearon]

This is more naïve than Lodash impl. For example this will return true for arrays.

[jdalton]

Naw that's straight up Lodash's implementation. It's straight copypasta.

[gaearon]

Oh, my bad. Thanks for correcting!

[gaearon]

because the ultimate goal would be to remove all production dependencies.

Why is this a goal?

[gaearon]

cc @jdalton.
IMO if there is an issue with Lodash, we should file it there rather than try to work around it here.

[paularmstrong]

because the ultimate goal would be to remove all production dependencies.

Why is this a goal?

Needing to pull in an extra huge dependency for such a small library doesn't seem totally necessary.

As for filing at lodash, I'm not totally certain there's a real problem there either, and we haven't found a repro using just lodash. It's also localized to Safari only, so the source of the problem is in Safari/Webkit's GC. We're working on filing a radar for the issue.

[gaearon]

Needing to pull in an extra huge dependency for such a small library doesn't seem totally necessary.

What is huge? If we need a deep equality check, presumably implementing it in house would be no smaller than Lodash’s implementation. We don’t pick any other parts of Lodash we don’t need—it’s not like we depend on the library itself. We depend on individual modules, only they will get bundled by the app.

[paularmstrong]

We could keep lodash if you really want, but I'm still not convinced on using mapValues, as the replacement solution is short and sweet.

Also, when I built from the repro gist provided, all of lodash is pulled in from normalizr, not just the methods needed.

[gaearon]

Also, when I built from the repro gist provided, all of lodash is pulled in from normalizr, not just the methods needed.

I think it’s not all of Lodash—it’s all the modules required by isEqual impl. It spans multiple modules but it’s still far from all of Lodash.

[paularmstrong]

Alright, I can revert to use lodash for isObject.

Still concerned about mapValues, though?

[gaearon]

Yea I don’t have a strong opinion on mapValues. Fine with using reduce there. But we shouldn’t remove Lodash just for the sake of removing Lodash. We cherry-pick only the modules we need and I don’t see the point of maintaining small-but-easy-to-get-it-wrong modules like isObject.

[jdalton]

Lodash offers utils, folks repeatedly rewrite them. I don't get it 😕

[gaearon]

I don’t want to rewrite anything or maintain it 😄

@paularmstrong Can you help me understand why pulling out Lodash and using the same implementation solves the issue in Safari?

[gaearon]

It appears like the only actual change here would be replacing mapValues with reduce. Is this what solves the Safari issue? Otherwise we're still using Lodash’s isEqual, and the isObject implementation you added seems to match Lodash’s.

[paularmstrong]

@gaearon yes. Replacing mapValues solves the issue. It's very hard to pinpoint whether the issue is in lodash or in the combination of normalizr and lodash at the moment.

This patch is in the interest of fixing what may be a pretty prevalent issue for those using normalizr first, since it's where the issue is cropping up.

[alunny]

Here is the repro in a jsbin: https://output.jsbin.com/coquxaduhu

If you open in Safari, without inspector open (which frustratingly appears to affect the behavior), you will see "missing entities.tweets" logged after some time. It's nondeterministic. We haven't been able to repro in any other browser.

We're working on isolating the error and reporting to Apple, but until then, empirically, @paularmstrong's patch causing the error to stop happening.

[gaearon]

Btw I appreciate both @paularmstrong’s help maintaining this repo and @jdalton’s help maintaining all the utilities I never want to write again 😄 . I’m sure we’ll find a solution but if it affects Lodash’s mapValues implementation we should really find a way to fix it upstream because it affects tons of other packages too in this case.

[jdalton]

If you open in Safari, without inspector open (which frustratingly appears to affect the behavior), you will see "missing entities.tweets" logged after some time. It's nondeterministic.

That's usually an indicator of a JIT bug not GC.

The mapValues method is super light compared to the work done by isEqual which recursively crawls objects and tracks visited ones (lots more things for a GC to get grumpy about)

[gaearon]

Confirmed I can reproduce this with Safari in JSBin posted above.

[jdalton]

This is less likely a Lodash issue and more likely using built-in Array#reduce disqualifies it from some JIT optimizations (like inlining) so bypasses the bug.

Previous versions of Safari had a JIT issue with .length which effected jQuery, Underscore, Lodash.
@stefanpenner did you hit something similar?

[gaearon]

I’d like to keep this open for a few more hours if you’d like to investigate more. I think we should release a fix in the evening since it consistently helps at least a couple of people but it’s not really ideal.

[jdalton]

Is there a way to get a reduced repro (without all the bundled stuff). Usually these things can come down to doing something in a tight loop one way then after the loop providing different input and it returning the wrong result.

For example:

QUnit.test('should iterate over an object with numeric keys (test in Mobile Safari 8)', function(assert) {
  assert.expect(1);

  // Trigger a Mobile Safari 8 JIT bug.
  // See https://github.com/lodash/lodash/issues/799.
  var counter = 0,
      object = { '1': 'foo', '8': 'bar', '50': 'baz' };

  lodashStable.times(1000, function(assert) {
    _.filter([], alwaysTrue);
  });

  _.filter(object, function() {
    counter++;
    return true;
  });

  assert.strictEqual(counter, 3);
});

[jdalton]

@paularmstrong
It would help to know how you're calling mapValues (what kind of input params).

[paularmstrong]

It would help to know how you're calling mapValues (what kind of input params).

@jdalton Plain JSON object, curried function wrapper

[jdalton]

Does the object have a length property or numeric keys?
Could you provide a small sample of the object and the curried function.

\cc @dmethvin @megawac New Safari JIT bug.

[necolas]

Friendly reminder that we're using this in production on mobile.twitter.com and it's causing production issues for Safari users. This patch to normalizr is trivial and a simplification with no apparent loss of functionality.

I think it’s not all of Lodash

Paul is pointing to the fact that when you install normalizr, it installs the whole of lodash.

[jdalton]

@necolas The JIT issue has been around for a few weeks (or that's how long I've heard grumbles about it) though no ones produced a simplified repro. Lodash is prepping for a version bump tonight so if you help create a simplified repro it'd be fixed in hours.

[dmethvin]

Apple just announced the Safari Technology Preview, maybe that will lift the curtain a bit and we can at least see if their canary-like build fixes this.

[gaearon]

I’ll cut a patch release with this to help reduce the number of affected users.

[jdalton]

I guess I'll punt too then since this will likely not be revisited after the merge 😩

[rniwa]

Could someone describe to me what the symptom here is? Is the issue that values in Object.keys don't return all keys? Can I have a pointer to the actual implementation of mapValues? I can't find it anywhere.

[gaearon]

@jdalton Yeah in this case we likely won’t be revisiting. Thanks for your help!

[jdalton]

@rniwa mapValues (_baseForOwn, _createBaseFor, keys, & _baseKeys).

Edit: Resolved below.

[gaearon]

The interim fix is out in 2.0.1. Based on reports in this thread it is enough to work around the issue although obviously can’t give any guarantees.

cc @necolas

[necolas]

Thanks for the quick turn around

Yeah but that’s just how npm works.

Yep :)

[rniwa]

@jdalton : Thanks!

[megawac]

Yeah but that’s just how npm works. Aside from a bigger download in node_modules, I don’t really see any issues with this.

You can consider using the lodash-modularized npm suite to avoid installing all of lodash

[gaearon]

Sorry, I just don’t see why bother. The result is identical both ways. User is more likely to already have lodash package than any of the modularized ones.

Anyway, if you’d like to discuss this, I welcome you to open a new issue. Let’s keep this thread focused on the Safari GC bug. Thanks!

[jdalton]

@rniwa
I modified the repro to remove the baseIteratee call from mapValues and that seems to fix it.
The baseIteratee module is here. Could it be related to the type juggling?

[gaearon]

Another instance of the same issue: gaearon/react-proxy#55

[gaearon]

the Safari JIT (I'm running Safari 9.0.3 (build 11601.4.4)) makes baseIteratee (called from lodash.find) randomly think that the find predicate ([key]) => key === Component -- is an Object and not a Function.

[jdalton]

Changing the typeof check in baseIteratee to a heavier isFunction check avoid the JIT bug. I don't want to do that though so I'll poke around a bit more but that looks like the issue.

[paularmstrong]

Thanks for the help and extra digging, everyone!

[jdalton]

Simplifying baseIteratee to:

function baseIteratee(value) {
  var type = typeof value;
  if (type == 'function') {
    return value;
  }
  return identity;
}

Still repros the JIT bug while removing the variable assignment avoids the bug:

function baseIteratee(value) {
  if (typeof value == 'function') {
    return value;
  }
  return identity;
}

[jdalton]

@rniwa The quick fix is to just use two typeofchecks instead of storing the result in a variable. However there are several other places in Lodash that store typeof results in variables. So, when the issue is narrowed down if there's a better workaround or a more clear root cause please let me know.

[rniwa]

Thanks a lot everyone for the bug report! With the information provided on this issue and gaearon/react-proxy#55, I've determined that the bug is caused by our DFG JIT and filed a WebKit bug 156034. Subsequently, my colleague was able to pinpoint the culprit to a single line of code and he's working on a fix.

[jdalton]

@rniwa Sweet! Do you know of a reduced repro for this?

[rniwa]

Here's a minimum reproduction for those who are interested (credit goes to Saam): https://gist.github.com/anonymous/49715c82e834f8e2c184d35a821ec468

Yes, this is a bug in typeof. It can return "object" for a Function. I think you can use x instanceof Function since the latter wouldn't suffer from the same bug.

[alunny]

@rniwa thanks for following up so quickly, very much appreciated :)

[jdalton]

Here's a simpler repro without varying the input type:
https://jsbin.com/kimita/1/

function foo(arg) {
  var o;
  if (arg) {
    o = function() {}
  } else {
    o = {};
  }
  return typeof o;
}

for (var i = 0; i < 300; i++) {
  log(foo(true));
}

[rniwa]

By the way, this bug is caused by our optimizing compiler generating wrong machine code, and nothing to do with GC on the contrary to what the title says.

[jdalton]

This fix made it into Lodash v4.7.0 due out in the morning as you all don't want me publishing before I 💤.

BTW this could have also been avoided by using webpack to reroute baseIteratee module requests to identity. It would have also produced a smaller build without reimplementing methods.

[justsml]

Thx @jdalton @rniwa et al.. I didn't believe the ghost stories about this bug!

[tinfoil-globe]

Thanks everyone! This was a crazy heisenbug.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.