Currently the RemotePageManager injects a bunch of functions into pages that register to use the RemotePageManager. One of those functions is 'sendAsyncMessage'. In our tree we already have quite so many sendAsyncMessage functions so it's very hard to determine whether sendAsyncMessage is used in the context of RPM or for any other mechanism, e.g. message passing between child and parent. Prepending the exported function with RPM allows to grep for RPMSendAsyncMessage which allows for easier security audits. See e.g: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/RemotePageManager.jsm#409-420

In fact it's not so easy to distinguish injected sendAsyncMessage from others sendAsyncMessage's within our tree. But probably exactly because of that this exercise to prepend all injected code with "RPM" makes sense. Here is a first try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4f1c1b86d67fb07948fb9ec486756a4e4552e6f

Hey Dave, as discussed within Bug 1459204 it makes sense to prepend all exported (or better injected) functions from RemotePageManager with "RPM". I've updated the code but I can't figure out what's happening with activity-stream. In particular, the following tests (see also [1]) are failing: * browser/extensions/activity-stream/test/functional/mochitest/browser_as_render.js * browser/extensions/activity-stream/test/functional/mochitest/browser_highlights_section.js * browser/extensions/activity-stream/test/functional/mochitest/browser_topsites_contextMenu_options.js * browser/extensions/activity-stream/test/functional/mochitest/browser_topsites_section.js I grepped for sendAsyncMessage, addMessageListener, and also removeMessageListener but it seems I can't find the right function call that needs to be updated. Do you happen to know what I could be missing? I am happy to debug more if you could provide some guidance on how I could figure out where the problem here is. Last question, might this change introduce some other problems that might occur outside of mozilla-central codebase? [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd64b95f0efae1464cab9c0a1b01d13c1ecfecdd&selectedJob=182519828

You also need to update the code in browser/extensions/activity-stream/content-src to use the new names. I guess they're using RPM already!

Hey Dave, a few notes about the patch: I am fairly confident about the changes, except the changes within browser/extensions/activity-stream, those were mostly try and error. I get a green TRY run[1], but I am not sure if we are just not exercising all of the code paths for activity stream. I think we should land this not before the next merge date (6/25) so we don't have to uplift in case we encounter problems. In particular, there is: * activity-stream.bundle.js.map, which also includes sendAsyncMessage, but I am not sure if it's related to the sendAsyncMessage of RPM. * test/unit/content-src/lib/init-store.test.js, which seems does not run on TRY but also includes global.sendAsyncMessage and also global.addMessageListener. * test/unit/content-src/lib/snippets.test.js, which also does global.addMessageListener. Can/Should we have someone from the activity stream folks also take a look at the patch? Finally, the top of * browser/extensions/activity-stream/data/content/activity-stream.bundle.js uses a different line ending, hence the difference in the patch, but no actual code changes besides the line ending. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce16f866bc5e68bdf282bd8685ef25c49a51dc76 Let me know what you think - thanks!

Comment on attachment 8986729 [details] [diff] [review] bug_1463663_prepend_rpm.patch Review of attachment 8986729 [details] [diff] [review]: ----------------------------------------------------------------- r+ for everything except the activity stream changes as as you say I think we should get someone from that team to review this. I think what is going on is that we have a copy of their source plus a transpiled copy of their code and maybe a source map too. Either way I'd like someone from that team to give the final stamp.

Comment on attachment 8986729 [details] [diff] [review] bug_1463663_prepend_rpm.patch >+++ b/browser/extensions/activity-stream/data/content/activity-stream.bundle.js >@@ -386,38 +386,37 @@ var actionUtils = { >-var g; >+var g; You could just exclude this chunk from the patch, but this is fine. It'll just get overwritten/reverted in the next export. I applied the patch and did `npm run bundle` from activity-stream directory, and the only diffs were for the line ending changes and activity-stream.bundle.js.map (where I guess having the sourcemap out of sync until next export is fine). Our unit tests are failing from the rename, but we'll just fix that as part of our porting this change to main repository: ``` FAILED TESTS: activity-stream ASRouterUtils ✖ should send a message with the right payload data global.RPMSendAsyncMessage is not a function sendTelemetry@webpack:///content-src/asrouter/asrouter-content.jsx:50:4 <- test/unit/unit-entry.js:14981:5 ASRouterUISurface ✖ should log errors from failed messages global.addMessageListener.firstCall is null @webpack:///test/unit/content-src/lib/init-store.test.js:49:0 <- test/unit/unit-entry.js:99785:11 SnippetsMap #getTotalBookmarksCount ✖ should dispatch a TOTAL_BOOKMARKS_REQUEST and resolve with the right data expected spy to be called with arguments @webpack:///test/unit/content-src/lib/snippets.test.js:156:0 <- test/unit/unit-entry.js:100225:7 SnippetsProvider #init(options) ✖ should add a message listener for incoming messages expected spy to be called with arguments @webpack:///test/unit/content-src/lib/snippets.test.js:278:0 <- test/unit/unit-entry.js:100347:7 ```

Carrying over r+ from mossop and mardak. (In reply to Ed Lee :Mardak from comment #7) > Comment on attachment 8986729 [details] [diff] [review] > bug_1463663_prepend_rpm.patch > > >+++ b/browser/extensions/activity-stream/data/content/activity-stream.bundle.js > >@@ -386,38 +386,37 @@ var actionUtils = { > >-var g; > >+var g; > You could just exclude this chunk from the patch Sure, I have done that for this version of the patch. > I applied the patch and did `npm run bundle` from activity-stream directory, > and the only diffs were for the line ending changes and > activity-stream.bundle.js.map (where I guess having the sourcemap out of > sync until next export is fine). Thank you. I suppose you have seen comment 5 in which case I am going to land this patch as is next week, after the merge date (6/25) so we can fix potential problems that might still occur without having to uplift any changes. Thanks everyone!

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3977af8a9fd Prefix exported functions of the RemotePageManager with RPM. r=mossop

Commits pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/b084c10ddd198b6bbddb81fd7d2e10f433a6d484 Backport Bug 1463663 - Rename sendAsyncMessage to RPMSendAsyncMessage https://github.com/mozilla/activity-stream/commit/c8dda028d13d473d5b954cf58bdbf7581f53a704 Merge pull request #4219 from piatra/backport1463663 Backport Bug 1463663 - Rename add/sendMessage to RPMAdd/Send