Review notes from Standard8

[Standard8]

I've taken a high level look at the min-vid add-on looking at compatibility issues with Firefox, security & performance issues.

I've not looked at the ctypes code, I'm trying to find someone who might be able to look at that.

Here's what I've picked up on:

• Starting the webextension in startup() will cause performance regressions in startup times. There's an example here of how you can avoid the performance regression. Screenshots also uses a similar mechanism, may be slightly more up to date.
• There is polling going on to pass messages between the content & chrome code (bootstrap.js & default.html), it would be better for performance & possibly security, if that used something like WebChannel if possible. I'm also concerned that as-implemented, it could loose messages when the temporary object gets reset.
• As far as I can tell, the min-vid window is loaded as unpriviledged due to the use of the resource uri, however, the video is being loaded in the main process. It would be nicer for performance if we could avoid this.

These are optional, based on recent changes in Firefox, and would probably not noticeably affect performance.

• Which versions of Firefox does this need to target? If it is only more recent versions, I think you should consider updating eslint & eslint-plugin-mozilla and pick up the latest changes for going from Cu.import to ChromeUtils.import etc (eslint will warn about these).
• In bootstrap.js, consider using XPCOMUtils.defineLazyModuleGetters rather than XPCOMUtils.defineLazyModuleGetter.

I think someone mentioned about running performance tests, I'll file a separate issue with details for that.

[ghost]

I've not looked at the ctypes code, I'm trying to find someone who might be able to look at that.

This turned out to be me.

I can only see one issue, which is here in the Mac implementation, where we're trying to log the Windows lasterror. The rest of the topify implementation looks good, both in the usage of ctypes and in the actual platform calls themselves.

I didn't do a thorough review through the entire contents of the ostypes_*.jsm files, because there's a ton in there and we're using very very little of it here, so I focused on the few relevant parts. Everything in those within that subset looks to be in order.

[marniepw]

FYI: @johngruen, @clouserw, and @meandavejustice

[jaredhirsch]

Thanks for the ctypes review, @Matt-Howell! That code has always freaked me out a bit :-P

What's the right way to log the error on that line you pointed out?

[jaredhirsch]

@Standard8 about the messaging implementation, I struggled getting messaging to work at all, and fell back on polling as a bit of a desperation move. A lot of users have had success with min-vid over the course of its existence, and I'd probably rather avoid changing that part of the implementation, if possible. Do you think there's an easy and dependable alternative?

[Standard8]

@6a68 I think the WebChannel stuff should be fairly reliable, as we've used it elsewhere. @mikedeboer or @Mossop might be able to suggest better ways.

[ghost]

What's the right way to log the error on that line you pointed out?

I'm not actually sure. The setLevel call itself doesn't return anything, and I'm having a hard time finding documentation on it to see if it sets errno or something. Other similar methods on NSWindow do not appear to indicate failures in any useful way. It may be okay to just assume the call succeeded.

[jaredhirsch]

Thanks again for the feedback, all. Filing a couple of followup bugs to fix individual issues.

[meandavejustice]

@Standard8

Which versions of Firefox does this need to target? If it is only more recent versions, I think you should consider updating eslint & eslint-plugin-mozilla and pick up the latest changes for going from Cu.import to ChromeUtils.import etc (eslint will warn about these).

We are targeting 60, how recent was that change? Will it break in 60?

[jaredhirsch]

@Standard8 Ah, I remember now: we don't use WebChannel because it requires the scheme to be https. I think we'd have to wire up some kind of custom MessageManager thing. (It's been a good year and a half since I worked this out, but I dimly recall running into other major obstacles, maybe due to the fact that this XUL window is slightly different than the others?)

The polling-messaging implementation is battle-tested, though hacky looking. I'm not concerned about its stability.

As for the security concern, I'm not sure what the attack vector might be, but maybe a security peer could take a look?

I'm not sure how to address possible performance concerns, but it does seem to have worked fine while in test pilot.

I'd prefer to just keep this corner of the code as currently written. If you think this definitely needs to be changed for the shield study, do you have any bandwidth to help us come up with an alternative messaging implementation?

[Standard8]

@6a68, ah that's an interesting point. At this stage, I'd agree with passing it by a security peer, unfortunately I don't know about all the specific concerns here, but I know there are concerns if there's data passing between chrome & content. Unfortunately I also can't remember who I was involved in some of the discussions around that which I saw.

The performance of having the video run in the main process you might get away with. You could possibly load the remote content in a browser or iframe (I think) element with type="content" and that might go remote, but that would also likely break your message passing, so possibly not worth it at the moment.

[Mossop]

I think we can accept having the video in the main process for now, it's definitely something we'd like to fix if we were to ship to a wider audience though.

Can you point me to the message passing code that you're concerned about?

[ghost]

We've already had a security review from @pauljt at https://docs.google.com/document/d/1h8Jvk7QA5cN4hrMZhZGCaJXhovq6J1B_Og-UjD8k0P8/edit . Pinging here so he sees this thread though. Paul -- any changes we need to do before the Shield study from your POV?

[jaredhirsch]

@Mossop The message passing in question works via shared mutable global state:

Chrome -> content:

• The bootstrap code sets the React app's global state via overwriting mvWindow.wrappedJSObject.AppData
• The content is a React app that uses window.AppData as its backing data store, so it responds to changes automatically

Content -> Chrome:

• React appends to an array called window.pendingCommands
• Bootstrap code polls for changes to mvWindow.wrappedJSObject.pendingCommands
• If Bootstrap code sees new messages, it calls a content-side function, mvWindow.wrappedJSObject.resetCommands(), to clear the list of messages, then handles the message on the chrome side

I think this covers the basics. Feel free to poke at the shield-study branch and ping me if you have any other questions.

[Mossop]

The chrome -> content side seems ok, if a little hacky. I'd much rather you used postMessage to get that data across and into the content's privileged level safely, assuming it is just raw JS data.

The polling for content -> chrome is not really ok, particularly since this is running in the main process. There are multiple ways you could do this differently, window.postMessage ought to work, as would listening for a DOM event from content (see f.e. https://searchfox.org/mozilla-central/source/toolkit/content/browser-content.js#1107) or just injecting a function into the content page that it can call to pass along a message (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.exportFunction). Grab me on IRC if you need help exploring any of that.