I filed a PR with HTTPS Everywhere to use bind, but the devs want to make sure this is an intentional change before they merge it:

https://github.com/EFForg/https-everywhere/pull/14886

Fun.

My guess is that this is happening with native bindings, which is currently an experiment running on canary/dev.  Running with a bound object will indeed work.

At first, I thought this was failing because of grabbing the context based on the Holder()->CreationContext() property from the v8 callback, but it actually looks like it happens even earlier: gin won't run functions for a gin::Wrappable without an appropriate `this` parameter.

This makes sense when looking specifically at the implementation of a gin::Wrappable - it grabs the C++ `this` object based on the `this` passed into the v8 function, and throws an (unhelpful) error if it fails to convert.  This makes sense.  Unfortunately, it does look like this is a behavior change from before, where we would bind independent of the object [1].

Jeremy, I'm curious if you have any thoughts on this, and what the web platform does in general cases.  I know that, at least in many cases, we'd throw an error on the web platform for something like this too (e.g., document.getElementById.apply(null, ['foo']).  Is that the norm?

If so, it may be worth enforcing this rather than trying to work around it (which could lead to some very funny situations - e.g. chrome.storage.local.get.apply(chrome.storage.sync, ['key', function() {}])).  But I do wonder how prolific this type of aliasing is...

[1] https://chromium.googlesource.com/chromium/src/+/3f2878168051e6c780d7e7f77760dd0b54a71ee9/extensions/renderer/resources/storage_area.js#38

Web platform will throw if you try to apply things to the wrong type. Blink generally uses v8::Signature to do this (will reject receivers that aren't constructed from the correct FunctionTemplate), but does the check manually in a few edge cases.

And of course JavaScript doesn't bind by default, so you get craziness if you try and apply a function to an unexpected |this|.

It would be possible to make the function, bind it ourselves, and set it as a property of the instance (rather than of the prototype), if we want this to continue to work.

I think that, if we *can*, I'd like to make this a breaking change in the spirit of keeping with general web platform behavior, per #2 and #3 (and given this is totally undefined and unspecified behavior to begin with).

That said, there is the chance that enough things break here that we realize this is infeasible (it's not our deliberate intention to break the world :)).  jbroman@, can you think of any clever way we could UMA the breakages here related to extensions specifically?  (I can't, short of specifying some creative flag in a gin::Wrappable for it to log, but you're more familiar with that code than I am)

For the HTTPS Everywhere case, I'd recommend that they update to bind.  And note that updating to bind should work whether we keep the new behavior or decide it's worth the added investment to maintain strict backwards compatibility with the unspec'd behavior.

Yeah, you could pass something (a callback?) gin::ObjectTemplateBuilder, and then check in Dispatcher (or ArgumentHolder, or something like that) if the holder argument didn't convert properly (and if so, trigger the callback to do UMA stuff, or whatever).

Could try to be more specific and figure out *why* it was wrong, but that gets progressively tougher.

Either way it's not super clean, but could be done.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d00988cd192a7918e4d6e6559b8ac29dcfc27e45

commit d00988cd192a7918e4d6e6559b8ac29dcfc27e45
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Apr 19 15:45:11 2018

[Extensions Bindings] Provide types for script-visible gin::Wrappables

Provide typenames (through gin::Wrappable::GetTypeName()) for types
that can be visible to developer script (i.e., those types that are
exposed and documented as part of the API). This way, if the
developer uses a method without an appropriate object, a helpful
error will be thrown (rather than just "Illegal invocation.").

Bug:  821654 
Change-Id: I6a6f64ffe012143725f7f1cfa25eddb84c96b799
Reviewed-on: https://chromium-review.googlesource.com/1013051
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552026}
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/bindings/declarative_event.cc
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/bindings/declarative_event.h
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/chrome_setting.cc
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/chrome_setting.h
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/content_setting.cc
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/content_setting.h
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/gin_port.cc
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/gin_port.h
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/storage_area.cc
[modify] https://crrev.com/d00988cd192a7918e4d6e6559b8ac29dcfc27e45/extensions/renderer/storage_area_unittest.cc

The patch in #6 makes the error much more clear and should provide guidance to extensions that are running into this case.

I'm going to mark this as fixed for now, and hope that there are few enough of the cases where extensions were doing this that we can maintain the principled approach of requiring an appropriate `this` object.

 Issue 886910  has been merged into this issue.