Using exportInstance and importInstance goes against convention and causes issues

[mlissner]

Way back in 2013, @zestyping added a kind of strange mechanism to the RECAP Extensions. In it, he created a system for background scripts to call functions in content scripts, and vice versa.

https://github.com/freelawproject/recap-chrome/commit/afaee9aeaddf31f0f67ce131d68a0ec3a0a2e128

This has served us well, but I think I'm going to overhaul it, or at least somebody should. Here's why:

1. This goes against convention. The convention is that content scripts can communicate with background scripts via message passing, and that's it. The reason for this is security, and I've always been vaguely concerned that the way we do this isn't great for security.

2. The mechanism used here required that our background scripts also be listed as content scripts. This isn't ideal in itself (security issue?), but it also means that using things like the webRequest API, which can only be used in background scripts, becomes...difficult. Right now I want to use that API, but the only way I can do so is to put it in background.js, which is our only background script that's not also a content script.

3. I've always found this part of the code confusing. I think they're just above my JS knowledge, but they're doing some kind of magic that I've never really understood. I could probably figure it out, but wouldn't it be nice to just do it the normal way?

I'm not sure how hard this will be to rip out, and I'm also not sure it all has to happen at once. Right now, I'm focused on getting recap.js to be only a background script, so I can fix the issue with webRequest's above. That may provide a little sample of how hard this is, and whether we want to do roll out the fix extension-wide.

[mlissner]

So, reading over this and the docs a bunch tonight, I think I understand what exportInstance and importInstance are doing. The basic idea is that content scripts communicate with background scripts via message passing, but when a content script sends a message, it goes to any piece of code that's listening to messages. This means that everybody rolls their own method for filtering the messages so that only the right functions respond to them.

Here's an example explaining it a bit better: https://stackoverflow.com/questions/44804009/messages-intended-for-one-script-in-the-background-context-are-received-by-all

What importInstance and exportInstance solve, via some cleverness and low-level JS work, is the need for every function in every background script to only responds to messages that have a name attribute that matches their own. And at the same time, this sets up content scripts to send messages with the name attribute properly completed. That's cool, and it lets you do things like this:

recap.some_function(args, callback)

Which sends a message like:

chrome.runtime.sendMessage(
    {name: recap, verb: some_function, args: args}, callback)

Which would be picked up only by the some_function function. Cool!

Alas, the way it does this is by that low-level JS, which has to be run in the content script using the background script as an argument. For example, this line is in content_delegate.js right now:

  this.recap = importInstance(Recap);

That's no good because Recap is in a background script, but to do the clever JS stuff, you need to be able to use Recap as an argument in your content script (as above).

So...I still think these have to go, even if they are a (pretty) elegant solution to this problem.

In their place, I think we'll want to roll our own message/response system, as explained by the Stackoverflow question above. I'll try to do a survey of solutions folks have for this in the wild. I'd love not to roll our own.

[avery-bub]

One approach that (in my opinion) could be slightly cleaner than the approach outlined here would be to leave the overall Recap() function intact and just call those functions from a single listener below. For example:

chrome.runtime.onMessage.addListener(function(request, sender, cb){

  if (request.package !== 'recap'){
    return;
  }

  let args = request.args;
  var recap = Recap();

  switch (request.function) {

    case 'uploadDocument':
     recap.uploadDocument(args.pacer_court, args.pacer_case_id, args.pacer_doc_id,
      args.document_number, args.attachment_number, args.bytes, cb);

    case 'somethingElse':
     ...

  }
});

Some benefits to this could be:

1. Less change to the current structure of the files (smaller chance of inadvertently introducing bugs).
2. Functions get to stay as functions instead of blocks of code within a bunch of if statements.

I realize that it's pretty much just a preference thing with no tangible difference in functionality.

[avery-bub]

So Mike noted that our approaches should strive to be more general. Since recap.js is not the only file that needs to remove exportInstance and importInstance, we should attempt to minimize the amount of similar code across files. I may have found such a way (although it seems a little odd).

I started by noticing that the problematic files/functions (Recap and Notifier) return a javascript object where the "keys" are the function names. Because of this, we should be able to pass this object as an argument to a general listener function as described below. First, I added a basic function to Recap() to facilitate easy testing (the other functions are hard for me to test with since I'm only vaguely aware of their overall purpose):

testListener: function(a, b, cb) {
      return cb(b - a);
 }

testListener is one of many functions returned in the Recap object. Next, I create the general listener class in a new file (listener.js):

function Listener(constructor) {
    var functionNames = Object.keys(constructor.functions),
        package = constructor.package;

    return function(request, sender){
        if (request.package != package) {
            return;
        }
        if (functionNames.indexOf(request.function) == -1) {
            return;
        }

        var args = request.args;
        let executeFunc = constructor.functions[request.function];

        return executeFunc.apply(this, args);
    }
}

Like before, we will add a listener at the bottom of a file like recap while keeping the overall Recap() object definition intact. However, instead of a bunch of switch statements to check for all possible function names, the only thing we must do to create the listener is the following:

var recap = Recap();
var constructor = {};
constructor['functions'] = recap;
constructor['package'] = 'recap';
chrome.runtime.onMessage.addListener(Listener(constructor))

Now, to send a message to this listener, we could do the following:

var payload = {};
payload['package'] = 'recap';
payload['function'] = 'testListener';
var easyFunc = function(num) {
    return(num * 2);
}
payload['args'] = [1, 4, easyFunc]
chrome.runtime.sendMessage(payload);

Using the above code, we have a relatively simple way to send a message to listeners. In this case, our listener would take the arguments and return 6. A few notes:

1. The 'args' of the payload are just the arguments for the function specified by the 'function' value. They must be in order here, but it would be possible to go back to the {argument_name: argument_value} structure we had before if necessary.

2. The callback function is now treated as an argument to be consistent (meaning that it is no longer sent in sendMessage). This, of course, can also change.

[mlissner]

This looks pretty good and feels pretty simple. Definitely getting pretty warm, and it avoids repeating just about anything, which is really nice.

Simplifying your explanation (and reiterating it in my own words so you can correct me if I misunderstand):

1. We keep our existing object format with Recap() and Notifier() and whatever, with function names as keys.

2. Whenever we have one of those, we also have to define a listener for it by doing:

   chrome.runtime.onMessage.addListener(Listener({
       'functions': Recap(),
       'package': 'recap'
   }))

3. Then to use it, we have to send a message with parameters for args, package, and function, with the last argument in args as the callback to be called at the end.

The only thing that seems weird to me is the callback as an argument bit, but I guess that's kind of where we're at already where it's always the final argument to each function. Seems nicer though to do:

chrome.runtime.sendMessage(payload, cb); 

Or:

chrome.runtime.sendMessage({
    'args': [1, 4],
    'package': 'recap',
    'function': 'testListener',
    'callback': easyFunc
});

Or something like that. That's still a bit wordier than what we've got now:

recap.testListener(1, 4, easyFunc);

Though perhaps that's just the way it has to be. It's certainly more explicit and less magic, but we're going from one line to 6, at best. But regardless, it does make sense to me not to have the callback as the last argument to the args array.

[avery-bub]

Yes, your understanding of (1) and (2) is correct. As for (3), I see what you are saying. It should be pretty trivial to change it so that the callback is no longer in the args array. I think I am partial toward changing it to look like this suggestion:

chrome.runtime.sendMessage({
    'args': [1, 4],
    'package': 'recap',
    'function': 'testListener',
    'callback': easyFunc
});

[mlissner]

Sounds good!

[mlissner]

We should do this as part of Manifest V3 upgrades, not before. Labeling accordingly.

[mlissner]

I believe we can close this too, @ERosendo?

[ERosendo]

Yes @mlissner, The exportInstance and importInstance methods have been removed in https://github.com/freelawproject/recap-chrome/pull/387

[mlissner]

Thank goodness.