Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Add cross browsers smoke tests and add support for sendMessage rejected promise behavior #115

Merged
merged 3 commits into from Jun 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions .travis.yml
Expand Up @@ -2,7 +2,8 @@ language: node_js
sudo: false
node_js:
## Some of the ES6 syntax used in the browser-polyfill sources is only supported on nodejs >= 6
- '6'
## and the selenium-webdriver dependency used by the integration tests requires nodejs >= 8.
- '8'

script:
- npm run build
Expand All @@ -14,7 +15,14 @@ script:
- export DISPLAY=:99.0
- sh -e /etc/init.d/xvfb start
- echo "RUN integration tests on chrome" &&
TRAVIS_CI=true ./test/run-chrome-smoketests.sh
TRAVIS_CI=true ./test/run-browsers-smoketests.sh

## See https://docs.travis-ci.com/user/chrome
sudo: required

addons:
firefox: 'latest'
chrome: 'stable'

after_script: npm run publish-coverage

Expand Down
14 changes: 11 additions & 3 deletions package.json
Expand Up @@ -22,9 +22,13 @@
"babel-plugin-transform-es2015-modules-umd": "^6.24.1",
"babel-preset-babili": "^0.0.10",
"babel-preset-es2017": "^6.24.1",
"browserify": "^16.2.2",
"chai": "^3.5.0",
"chromedriver": "^2.38.3",
"eslint": "^3.9.1",
"finalhandler": "^1.1.0",
"geckodriver": "^1.11.0",
"global-replaceify": "^1.0.0",
"grunt": "^1.0.1",
"grunt-babel": "^6.0.0",
"grunt-contrib-concat": "^1.0.1",
Expand All @@ -35,9 +39,13 @@
"jsdom": "^9.6.0",
"mocha": "^3.1.0",
"nyc": "^8.3.1",
"puppeteer": "^0.10.2",
"selenium-webdriver": "^4.0.0-alpha.1",
"serve-static": "^1.13.1",
"sinon": "^1.17.6"
"shelljs": "^0.8.2",
"sinon": "^1.17.6",
"tap-nirvana": "^1.0.8",
"tape-async": "^2.3.0",
"tmp": "0.0.33"
},
"nyc": {
"reporter": [
Expand All @@ -54,6 +62,6 @@
"test": "mocha",
"test-coverage": "COVERAGE=y nyc mocha",
"test-minified": "TEST_MINIFIED_POLYFILL=1 mocha",
"test-integration": "mocha -r test/mocha-babel test/integration/test-*"
"test-integration": "tape test/integration/test-*"
}
}
104 changes: 86 additions & 18 deletions src/browser-polyfill.js
Expand Up @@ -7,6 +7,13 @@
"use strict";

if (typeof browser === "undefined") {
const SEND_RESPONSE_DEPRECATION_WARNING = `
Returning a Promise is the preferred way to send a reply from an
onMessage/onMessageExternal listener, as the sendResponse will be
removed from the specs (See
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/onMessage)
`.replace(/\s+/g, " ").trim();

// Wrapping the bulk of this polyfill in a one-time-use function is a minor
// optimization for Firefox. Since Spidermonkey does not fully parse the
// contents of a function until the first time it's called, and since it will
Expand Down Expand Up @@ -97,6 +104,8 @@ if (typeof browser === "undefined") {
};
};

const pluralizeArguments = (numArgs) => numArgs == 1 ? "argument" : "arguments";

/**
* Creates a wrapper function for a method with the given name and metadata.
*
Expand All @@ -120,8 +129,6 @@ if (typeof browser === "undefined") {
* The generated wrapper function.
*/
const wrapAsyncFunction = (name, metadata) => {
const pluralizeArguments = (numArgs) => numArgs == 1 ? "argument" : "arguments";

return function asyncFunctionWrapper(target, ...args) {
if (args.length < metadata.minArgs) {
throw new Error(`Expected at least ${metadata.minArgs} ${pluralizeArguments(metadata.minArgs)} for ${name}(), got ${args.length}`);
Expand Down Expand Up @@ -338,6 +345,9 @@ if (typeof browser === "undefined") {
},
});

// Keep track if the deprecation warning has been logged at least once.
let loggedSendResponseDeprecationWarning = false;

const onMessageWrappers = new DefaultWeakMap(listener => {
if (typeof listener !== "function") {
return listener;
Expand Down Expand Up @@ -366,12 +376,21 @@ if (typeof browser === "undefined") {
let wrappedSendResponse;
let sendResponsePromise = new Promise(resolve => {
wrappedSendResponse = function(response) {
if (!loggedSendResponseDeprecationWarning) {
console.warn(SEND_RESPONSE_DEPRECATION_WARNING, new Error().stack);
loggedSendResponseDeprecationWarning = true;
}
didCallSendResponse = true;
resolve(response);
};
});

let result = listener(message, sender, wrappedSendResponse);
let result;
try {
result = listener(message, sender, wrappedSendResponse);
} catch (err) {
result = Promise.reject(err);
}

const isResultThenable = result !== true && isThenable(result);

Expand All @@ -382,36 +401,85 @@ if (typeof browser === "undefined") {
return false;
}

// A small helper to send the message if the promise resolves
// and an error if the promise rejects (a wrapped sendMessage has
// to translate the message into a resolved promise or a rejected
// promise).
const sendPromisedResult = (promise) => {
promise.then(msg => {
// send the message value.
sendResponse(msg);
}, error => {
// Send a JSON representation of the error if the rejected value
// is an instance of error, or the object itself otherwise.
let message;
if (error && (error instanceof Error ||
typeof error.message === "string")) {
message = error.message;
} else {
message = "An unexpected error occurred";
}

sendResponse({
__mozWebExtensionPolyfillReject__: true,
message,
});
}).catch(err => {
// Print an error on the console if unable to send the response.
console.error("Failed to send onMessage rejected reply", err);
});
};

// If the listener returned a Promise, send the resolved value as a
// result, otherwise wait the promise related to the wrappedSendResponse
// callback to resolve and send it as a response.
if (isResultThenable) {
result.then(sendResponse, error => {
console.error(error);
// TODO: the error object is not serializable and so for now we just send
// `undefined`. Nevertheless, as being discussed in #97, this is not yet
// providing the expected behavior (the promise received from the sender should
// be rejected when the promise returned by the listener is being rejected).
sendResponse(undefined);
});
sendPromisedResult(result);
} else {
sendResponsePromise.then(sendResponse, error => {
console.error(error);
// TODO: same as above, we are currently sending `undefined` in this scenario
// because the error oject is not serializable, but it is not yet the behavior
// that this scenario should present.
sendResponse(undefined);
});
sendPromisedResult(sendResponsePromise);
}

// Let Chrome know that the listener is replying.
return true;
};
});

const wrappedSendMessageCallback = ({reject, resolve}, reply) => {
if (chrome.runtime.lastError) {
reject(chrome.runtime.lastError);
} else if (reply && reply.__mozWebExtensionPolyfillReject__) {
// Convert back the JSON representation of the error into
// an Error instance.
reject(new Error(reply.message));
} else {
resolve(reply);
}
};

const wrappedSendMessage = (name, metadata, apiNamespaceObj, ...args) => {
if (args.length < metadata.minArgs) {
throw new Error(`Expected at least ${metadata.minArgs} ${pluralizeArguments(metadata.minArgs)} for ${name}(), got ${args.length}`);
}

if (args.length > metadata.maxArgs) {
throw new Error(`Expected at most ${metadata.maxArgs} ${pluralizeArguments(metadata.maxArgs)} for ${name}(), got ${args.length}`);
}

return new Promise((resolve, reject) => {
const wrappedCb = wrappedSendMessageCallback.bind(null, {resolve, reject});
args.push(wrappedCb);
apiNamespaceObj.sendMessage(...args);
});
};

const staticWrappers = {
runtime: {
onMessage: wrapEvent(onMessageWrappers),
onMessageExternal: wrapEvent(onMessageWrappers),
sendMessage: wrappedSendMessage.bind(null, "sendMessage", {minArgs: 1, maxArgs: 3}),
},
tabs: {
sendMessage: wrappedSendMessage.bind(null, "sendMessage", {minArgs: 2, maxArgs: 3}),
},
};

Expand Down
12 changes: 12 additions & 0 deletions test/fixtures/browserify-tape.js
@@ -0,0 +1,12 @@
const browserify = require("browserify");

const b = browserify();

b.add("./test/fixtures/tape-standalone.js");
b.transform("global-replaceify", {
global: true,
replacements: {
setImmediate: "require('timers').setImmediate",
},
});
b.bundle().pipe(process.stdout);
@@ -0,0 +1,18 @@
test("browser api object in content script", (t) => {
t.ok(browser && browser.runtime, "a global browser API object should be defined");
t.ok(chrome && chrome.runtime, "a global chrome API object should be defined");

if (navigator.userAgent.includes("Firefox/")) {
// Check that the polyfill didn't create a polyfill wrapped browser API object on Firefox.
t.equal(browser.runtime, chrome.runtime, "browser.runtime and chrome.runtime should be equal on Firefox");
// On Firefox, window is not the global object for content scripts, and so we expect window.browser to not
// be defined.
t.equal(window.browser, undefined, "window.browser is expected to be undefined on Firefox");
} else {
// Check that the polyfill has created a wrapped API namespace as expected.
t.notEqual(browser.runtime, chrome.runtime, "browser.runtime and chrome.runtime should not be equal");
// On chrome, window is the global object and so the polyfilled browser API should
// be also equal to window.browser.
t.equal(browser, window.browser, "browser and window.browser should be the same object");
}
});
@@ -0,0 +1,19 @@
{
"manifest_version": 2,
"name": "test-detect-browser-api-object-in-content-script",
"version": "0.1",
"description": "test-detect-browser-api-object-in-content-script",
"content_scripts": [
{
"matches": [
"http://localhost/*"
],
"js": [
"browser-polyfill.js",
"tape.js",
"content.js"
]
}
],
"permissions": []
}
39 changes: 39 additions & 0 deletions test/fixtures/multiple-onmessage-listeners-extension/background.js
@@ -0,0 +1,39 @@
console.log(name, "background page loaded");

async function testMessageHandler(msg, sender) {
console.log(name, "background received msg", {msg, sender});

// We only expect messages coming from a content script in this test.
if (!sender.tab || !msg.startsWith("test-multiple-onmessage-listeners:")) {
return {
success: false,
failureReason: `An unexpected message has been received: ${JSON.stringify({msg, sender})}`,
};
}

if (msg.endsWith(":resolve-to-undefined")) {
return undefined;
}

if (msg.endsWith(":resolve-to-null")) {
return null;
}

return {
success: false,
failureReason: `An unexpected message has been received: ${JSON.stringify({msg, sender})}`,
};
}

// Register the same message handler twice.
browser.runtime.onMessage.addListener(testMessageHandler);
browser.runtime.onMessage.addListener(testMessageHandler);

// Register an additional message handler that always reply after
// a small latency time.
browser.runtime.onMessage.addListener(async (msg, sender) => {
await new Promise(resolve => setTimeout(resolve, 100));
return "resolved-to-string-with-latency";
});

console.log(name, "background page ready to receive a content script message...");
18 changes: 18 additions & 0 deletions test/fixtures/multiple-onmessage-listeners-extension/content.js
@@ -0,0 +1,18 @@
test("Multiple runtime.onmessage listeners which resolve to undefined", async (t) => {
const res = await browser.runtime.sendMessage("test-multiple-onmessage-listeners:resolve-to-undefined");

if (navigator.userAgent.includes("Firefox/")) {
t.deepEqual(res, undefined, "Got an undefined value as expected");
} else {
// NOTE: When an onMessage listener sends `undefined` in a response,
// Chrome internally converts it to null and the receiver receives it
// as a null object.
t.deepEqual(res, null, "Got a null value as expected on Chrome");
}
});

test("Multiple runtime.onmessage listeners which resolve to null", async (t) => {
const res = await browser.runtime.sendMessage("test-multiple-onmessage-listeners:resolve-to-null");

t.deepEqual(res, null, "Got a null value as expected");
});
25 changes: 25 additions & 0 deletions test/fixtures/multiple-onmessage-listeners-extension/manifest.json
@@ -0,0 +1,25 @@
{
"manifest_version": 2,
"name": "test-multiple-onmessage-listeners",
"version": "0.1",
"description": "test-multiple-onmessage-listeners",
"content_scripts": [
{
"matches": [
"http://localhost/*"
],
"js": [
"browser-polyfill.js",
"tape.js",
"content.js"
]
}
],
"permissions": [],
"background": {
"scripts": [
"browser-polyfill.js",
"background.js"
]
}
}
12 changes: 12 additions & 0 deletions test/fixtures/runtime-messaging-extension/background.js
Expand Up @@ -33,6 +33,18 @@ browser.runtime.onMessage.addListener((msg, sender, sendResponse) => {
// a reply from the second listener.
return false;

case "test - sendMessage with returned rejected Promise with Error value":
return Promise.reject(new Error("rejected-error-value"));

case "test - sendMessage with returned rejected Promise with non-Error value":
return Promise.reject("rejected-non-error-value");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try Promise.reject({message: "rejected-non-error-value"}) instead.

If you wish, keep the existing test case, but check that no useful error message is passed through.

Copy link

@james-cnz james-cnz May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If keeping the existing test case, maybe check specifically for the message "An unexpected error occurred"? (EDIT: I guess it's kind of an implementation detail, but I think it might be easier to do it this way.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, I think this mistake was already present in the code, not introduced by the pull request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added both the cases, to be fair I think that the behavior that Firefox have when an extension code rejects a plain object with a message property is actually an involuntary side-effect of the behavior we wanted when an API implementation rejects that kind of plain objects:

https://searchfox.org/mozilla-central/search?q=Promise.reject(%7Bmessage%3A&case=false&regexp=false&path=

Anyway, the polyfill is currently matching this behavior and I added an explicit test case for it.

@james-cnz Like I described in one of the above comments I'm not asserting the "An unexpected error occurred" error message because Firefox 59 and Firefox 60 have different behaviors (and so the test was failing on travis only on Firefox).


case "test - sendMessage with returned rejected Promise with non-Error value with message property":
return Promise.reject({message: "rejected-non-error-message"});

case "test - sendMessage with listener callback throws":
throw new Error("listener throws");

default:
return Promise.resolve(
`Unxpected message received by the background page: ${JSON.stringify(msg)}\n`);
Expand Down