Null check for calculateToString method #16478

lifeart · 2018-04-10T08:31:31Z

Issue found in: https://travis-ci.org/emberjs/ember-inspector/jobs/364445291

Object.getPrototypeOf(null)
// Uncaught TypeError: Cannot convert undefined or null to object
//     at Function.getPrototypeOf (<anonymous>)
//     at <anonymous>:1:8

TypeError: Cannot convert undefined or null to object
                at Function.getPrototypeOf (<anonymous>)
                at calculateToString (http://localhost:7357/testing/assets/vendor.js:44972:29)
                at Mixin.classToString [as toString] (http://localhost:7357/testing/assets/vendor.js:44890:12)
                at mixins.forEach.mixin (http://localhost:7357/testing/ember_debug.js:3015:26)
                at Array.forEach (<anonymous>)
                at Class.mixinsForObject (http://localhost:7357/testing/ember_debug.js:3007:14)
                at Class.sendObject (http://localhost:7357/testing/ember_debug.js:2923:26)
                at Class.inspect (http://localhost:7357/testing/ember_debug.js:2347:35)
                at Object.<anonymous> (http://localhost:7357/testing/assets/tests.js:4637:22)
                at runTest (http://localhost:7357/testing/assets/test-support.js:4286:30)

How to reproduce?

function foo() {"use strict"; return this };
classToString(foo);

rwjblue

Seems good, can you create a regression test here that roughly simulates what the inspector is doing?

rwjblue · 2018-04-10T13:16:10Z

Thank you for the added test! I'm still having trouble figuring out how this can actually happen though. Generally speaking, the following is being done:

class Foo {}
Foo.prototype.toString = classToString;

How is it, in this circumstance, that you would have the toString function without a this?

rwjblue · 2018-04-10T13:44:49Z

After a bit of digging, it seems like the ember-inspector is essentially doing this (in the test that was failing from the CI log above):

let obj = Ember.Object.create();
let mixins = Ember.Mixin.mixins(obj);
let mixinDetails = [];
mixins.forEach(mixin => {
  let name = mixin[Ember.NAME_KEY] || mixin.ownerConstructor;
  if (!name && typeof mixin.toString === 'function') {
    name = mixin.toString();
  }
  if (!name) {
    name = 'Unknown mixin';
  }
  mixinDetails.push({
    name: name.toString(), 
    properties: propertiesForMixin(mixin),
  });
});

Basically, what is going on is that the inspector is trying to call .toString() on each mixin and the current guard only checks for Function.prototype to break out of the loop which (since this is an instance at that point it would need to check for Object.prototype to break the loop).

rwjblue · 2018-04-10T13:48:15Z

packages/ember-metal/tests/namespace_search_test.js

+  class extends AbstractTestCase {
+    ['@test classToString: null as this inside class must not throw error'](assert) {
+      function F() {"use strict"; return this };
+      result = classToString(F.bind(null));


Can you update the regression test to do roughly:

let mixin = Mixin.create(); mixin.toString(); // without the fix, this throws

rwjblue · 2018-04-10T13:48:49Z

packages/ember-metal/lib/namespace_search.js

@@ -184,6 +184,9 @@ function calculateToString(target) {
    }
    let superclass = target;
    do {
+      if (superclass === null) {
+        break;
+      }
      superclass = Object.getPrototypeOf(superclass);
      if (superclass === Function.prototype) {


Can you update this to be:

if (superclass === Function.prototype || superclass === Object.prototype) { break; }

rwjblue · 2018-04-10T13:49:19Z

packages/ember-metal/lib/namespace_search.js

@@ -184,6 +184,9 @@ function calculateToString(target) {
    }
    let superclass = target;
    do {
+      if (superclass === null) {


If we add Object.prototype check (mentioned in another comment) we can remove the explicit null check..

rwjblue · 2018-04-10T13:51:52Z

Sorry for the run around here @lifeart, thank you for working on it!

lifeart · 2018-04-10T15:37:53Z

@rwjblue How is it, in this circumstance, that you would have the toString function without a this?
https://github.com/emberjs/ember-inspector/pull/781/files#diff-7c2c7ea1dfc0ef384d1ec6e685711d15R480

rwjblue · 2018-04-11T19:47:39Z

OK, looks great, would you mind squashing down to a single commit and prefixing the commit message with [BUGFIX release]?

@rwjblue

In general,Ember's testing system leverages `ajaxSend` and `ajaxComplete` events to know when `jQuery.ajax` requests are pending (and it should therefore "pause settledness" of the testing helpers like `click` / `andThen` / etc). In the Ember 3.1 cycle Ember changed from using `jQuery.on` to `document.addEventListener` to subscribe to these events. Unfortunately, `jQuery.ajax` **does not** trigger listeners added in this way (we would have had to do `document.onajaxSend = function() { /* stuff here */ }`). The result of this change is that "normal" acceptance testing helpers will _no longer_ be able to detect and wait for pending `jQuery.ajax`. So, for example, you might expect that given the following ```js await click('#some-selector'); assert.equal( $('#something-that-should-exist-after-an-ajax-call').text(), 'hi' ); ``` the click helper would wait until `jQuery.ajax` calls were settled before the assertion, but prior to the changes in this commit the promise returned by `click` would **not** wait until the `jQuery.ajax` request was completed and therefore the assertion would fail. --- This commit reverts the changes to use `document.addEventListener` back to using `jQuery.on` directly when `jQuery` is available. Many bothans died to bring us this information. 😭 😢 :crying_cat: Paired with @rwjblue to resolve this issue.

* Remove unused legacy global flags. * Move imports "to the top" * Bring back `Ember.NAME_KEY` * Ensure all `computed.*` exports are setup in reexports test * Have `ember-metal/libaries` register the `Ember` library

Fixes #16455

The refactor to move `index.js` -> `lib/index.js` left this in a broken state (the paths are incorrect and therefore the funnel threw an error at build time).

The shrug emoticon went from having a broken arm (improperly escaped) to no arm during the [prettifying](https://github.com/emberjs/ember.js/pull/16440/files#diff-437a149f1b0f7d248f37db51346c8ff2L156) process.

Issue found in: https://travis-ci.org/emberjs/ember-inspector/jobs/364445291

rwjblue requested changes Apr 10, 2018

View reviewed changes

lifeart mentioned this pull request Apr 10, 2018

Feature: ComputedProperty Dependent keys in debugger & services highlighting emberjs/ember-inspector#781

Merged

cibernox and others added 12 commits April 11, 2018 23:51

Remove Ember.required, marked as deprecated until 3.0.0

56e6ee2

Fix release branch name...

ae58c8d

Cleanup / validate global exports.

7fd58af

* Remove unused legacy global flags. * Move imports "to the top" * Bring back `Ember.NAME_KEY` * Ensure all `computed.*` exports are setup in reexports test * Have `ember-metal/libaries` register the `Ember` library

[CLEANUP beta] Override console.error with a noop function

ffe4776

update ember-cli to recent SHA

937bfd4

[DOCUMENTATION] Fix value in ApplicationInstance isBrowser property

186dda5

Fixes #16455

Fix paths for ember-source addon.

84d4031

The refactor to move `index.js` -> `lib/index.js` left this in a broken state (the paths are incorrect and therefore the funnel threw an error at build time).

[BUGFIX beta] Fix missing limb

11da5b0

The shrug emoticon went from having a broken arm (improperly escaped) to no arm during the [prettifying](https://github.com/emberjs/ember.js/pull/16440/files#diff-437a149f1b0f7d248f37db51346c8ff2L156) process.

[Feature] module unification service blueprint

9b19998

[CLEANUP beta] #16391 Cleaning up the test output

aabdd57

[BUGFIX release] Object check for calculateToString method

ec0a581

Issue found in: https://travis-ci.org/emberjs/ember-inspector/jobs/364445291

lifeart closed this Apr 11, 2018

This was referenced Apr 11, 2018

[Bugfix] object to string #16497

Closed

[bugfix] object to string serialization #16499

Merged

rwjblue mentioned this pull request Apr 16, 2018

[WIP] Trying to fix beta and canary emberjs/ember-inspector#787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Null check for calculateToString method #16478

Null check for calculateToString method #16478

lifeart commented Apr 10, 2018 •

edited

Loading

rwjblue left a comment

rwjblue commented Apr 10, 2018

rwjblue commented Apr 10, 2018

rwjblue Apr 10, 2018

rwjblue Apr 10, 2018

rwjblue Apr 10, 2018

rwjblue commented Apr 10, 2018

lifeart commented Apr 10, 2018

rwjblue commented Apr 11, 2018

Null check for calculateToString method #16478

Null check for calculateToString method #16478

Conversation

lifeart commented Apr 10, 2018 • edited Loading

rwjblue left a comment

Choose a reason for hiding this comment

rwjblue commented Apr 10, 2018

rwjblue commented Apr 10, 2018

rwjblue Apr 10, 2018

Choose a reason for hiding this comment

rwjblue Apr 10, 2018

Choose a reason for hiding this comment

rwjblue Apr 10, 2018

Choose a reason for hiding this comment

rwjblue commented Apr 10, 2018

lifeart commented Apr 10, 2018

rwjblue commented Apr 11, 2018

lifeart commented Apr 10, 2018 •

edited

Loading