Incorporate code review feedback from buschtoens.

* Extract shared utility for a `this` context that errors when used.
* Assert when more positional arguments are provided.
* Avoid wrapping callback needlessly (makes debugging a bit easier,
  since the event handler function is actually the user-land function in
  most cases.
* Incorporates a system to error if any `this` properties are
  accessed from within the callback in debug builds.
* Fixup comments for addEventListener / removeEventListener logic.
* Fix test assertion message.
* Add test ensuring the modifier is destroyed properly.

packages/@ember/-internals/glimmer/lib/helpers/fn.ts

@@ -1,43 +1,11 @@
-import { HAS_NATIVE_PROXY } from '@ember/-internals/utils';
 import { assert } from '@ember/debug';
-import { DEBUG } from '@glimmer/env';
 import { Arguments, VM } from '@glimmer/runtime';
 import { ICapturedArguments } from '@glimmer/runtime/dist/types/lib/vm/arguments';
 import { Opaque } from '@glimmer/util';
 import { InternalHelperReference } from '../utils/references';
+import buildUntouchableThis from '../utils/untouchable-this';
 
-let context: any = null;
-if (DEBUG && HAS_NATIVE_PROXY) {
-  let assertOnProperty = (property: string | number | symbol) => {
-    assert(
-      `You accessed \`this.${String(
-        property
-      )}\` from a function passed to the \`fn\` helper, but the function itself was not bound to a valid \`this\` context. Consider updating to usage of \`@action\`.`
-    );
-  };
-
-  context = new Proxy(
-    {},
-    {
-      get(_target: {}, property: string | symbol) {
-        assertOnProperty(property);
-      },
-
-      set(_target: {}, property: string | symbol) {
-        assertOnProperty(property);
-
-        return false;
-      },
-
-      has(_target: {}, property: string | symbol) {
-        assertOnProperty(property);
-
-        return false;
-      },
-    }
-  );
-}
-
+const context = buildUntouchableThis('`fn` helper');
 function fnHelper({ positional }: ICapturedArguments) {
   assert(
     `You must pass a function as the \`fn\` helpers first argument, you passed ${positional

packages/@ember/-internals/glimmer/lib/modifiers/on.ts

@@ -4,6 +4,9 @@ import { Opaque, Simple } from '@glimmer/interfaces';
 import { Tag } from '@glimmer/reference';
 import { Arguments, CapturedArguments, ModifierManager } from '@glimmer/runtime';
 import { Destroyable } from '@glimmer/util';
+import buildUntouchableThis from '../utils/untouchable-this';
+
+const untouchableContext = buildUntouchableThis('`on` modifier');
 
 /*
   Internet Explorer 11 does not support `once` and also does not support
@@ -105,21 +108,39 @@ export class OnModifierState {
       this.shouldUpdate = true;
     }
 
+    assert(
+      `You can only pass two positional arguments (event name and callback) to the \`on\` modifier, but you provided ${
+        args.positional.length
+      }. Consider using the \`fn\` helper to provide additional arguments to the \`on\` callback.`,
+      args.positional.length === 2
+    );
+
+    let needsCustomCallback =
+        (SUPPORTS_EVENT_OPTIONS === false && once) /* needs manual once implementation */ ||
+        (DEBUG && passive) /* needs passive enforcement */;
+
     if (this.shouldUpdate) {
-      let callback = (this.callback = function(this: Element, event) {
-        if (DEBUG && passive) {
-          event.preventDefault = () => {
-            assert(
-              `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${userProvidedCallback}`
-            );
-          };
-        }
-
-        if (!SUPPORTS_EVENT_OPTIONS && once) {
-          removeEventListener(this, eventName, callback, options);
-        }
-        return userProvidedCallback.call(null, event);
-      });
+      if (needsCustomCallback) {
+        let callback = (this.callback = function(this: Element, event) {
+          if (DEBUG && passive) {
+            event.preventDefault = () => {
+              assert(
+                `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${userProvidedCallback}`
+              );
+            };
+          }
+
+          if (!SUPPORTS_EVENT_OPTIONS && once) {
+            removeEventListener(this, eventName, callback, options);
+          }
+          return userProvidedCallback.call(untouchableContext, event);
+        });
+      } else if (DEBUG) {
+        // prevent the callback from being bound to the element
+        this.callback = userProvidedCallback.bind(untouchableContext);
+      } else {
+        this.callback = userProvidedCallback;
+      }
     }
   }
 
@@ -156,7 +177,7 @@ function removeEventListener(
     // used only in the following cases:
     //
     // * where there is no options
-    // * `{ once: true | false, passive: true, capture: false }
+    // * `{ once: true | false, passive: true | false, capture: false }
     element.removeEventListener(eventName, callback);
   }
 }
@@ -184,7 +205,7 @@ function addEventListener(
     // used only in the following cases:
     //
     // * where there is no options
-    // * `{ once: true | false, passive: true, capture: false }
+    // * `{ once: true | false, passive: true | false, capture: false }
     element.addEventListener(eventName, callback);
   }
 }

packages/@ember/-internals/glimmer/lib/utils/untouchable-this.ts

@@ -0,0 +1,39 @@
+import { HAS_NATIVE_PROXY } from '@ember/-internals/utils';
+import { assert } from '@ember/debug';
+import { DEBUG } from '@glimmer/env';
+
+export default function buildUntouchableThis(source: string): null | object {
+  let context: null | object = null;
+  if (DEBUG && HAS_NATIVE_PROXY) {
+    let assertOnProperty = (property: string | number | symbol) => {
+      assert(
+        `You accessed \`this.${String(
+          property
+        )}\` from a function passed to the ${source}, but the function itself was not bound to a valid \`this\` context. Consider updating to usage of \`@action\`.`
+      );
+    };
+
+    context = new Proxy(
+      {},
+      {
+        get(_target: {}, property: string | symbol) {
+          assertOnProperty(property);
+        },
+
+        set(_target: {}, property: string | symbol) {
+          assertOnProperty(property);
+
+          return false;
+        },
+
+        has(_target: {}, property: string | symbol) {
+          assertOnProperty(property);
+
+          return false;
+        },
+      }
+    );
+  }
+
+  return context;
+}

packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js

@@ -43,7 +43,7 @@ if (EMBER_GLIMMER_ON_MODIFIER) {
         if (isChrome || isFirefox) {
           assert.strictEqual(SUPPORTS_EVENT_OPTIONS, true, 'is true in chrome and firefox');
         } else if (isIE11) {
-          assert.strictEqual(SUPPORTS_EVENT_OPTIONS, false, 'is true in chrome and firefox');
+          assert.strictEqual(SUPPORTS_EVENT_OPTIONS, false, 'is false in IE11');
         } else {
           assert.expect(0);
         }
@@ -319,6 +319,52 @@ if (EMBER_GLIMMER_ON_MODIFIER) {
           this.render(`<button {{on 'click' this.foo}}>Click Me</button>`, { foo: null });
         }, /You must pass a function as the second argument to the `on` modifier/);
       }
+
+      '@test asserts if the provided callback accesses `this` without being bound prior to passing to on'() {
+        this.render(`<button {{on 'click' this.myFunc}}>Click Me</button>`, {
+          myFunc() {
+            expectAssertion(() => {
+              this.arg1;
+            }, /You accessed `this.arg1` from a function passed to the `on` modifier, but the function itself was not bound to a valid `this` context. Consider updating to usage of `@action`./);
+          },
+
+          arg1: 'foo',
+        });
+
+        runTask(() => this.$('button').click());
+      }
+
+      '@test asserts if more than 2 positional parameters are provided'() {
+        expectAssertion(() => {
+          this.render(`<button {{on 'click' this.callback this.someArg}}>Click Me</button>`, {
+            callback() {},
+            someArg: 'foo',
+          });
+        }, /You can only pass two positional arguments \(event name and callback\) to the `on` modifier, but you provided 3. Consider using the `fn` helper to provide additional arguments to the `on` callback./);
+      }
+
+      '@test it removes the modifier when the element is removed'(assert) {
+        let count = 0;
+
+        this.render(
+          '{{#if this.showButton}}<button {{on "click" this.callback}}>Click Me</button>{{/if}}',
+          {
+            callback() {
+              count++;
+            },
+            showButton: true,
+          }
+        );
+
+        this.assertCounts({ adds: 1, removes: 0 });
+
+        runTask(() => this.$('button').click());
+        assert.equal(count, 1, 'has been called 1 time');
+
+        runTask(() => this.context.set('showButton', false));
+
+        this.assertCounts({ adds: 1, removes: 1 });
+      }
     }
   );
 }