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

fix: Added missing chrome privacy api settings #205

Merged
merged 14 commits into from Nov 21, 2019

Conversation

Manvel
Copy link
Contributor

@Manvel Manvel commented Oct 14, 2019

This PR is for fixing #204

@Manvel Manvel changed the title #204 - Added missing chrome privacy api settings fix: Added missing chrome privacy api settings Oct 14, 2019
@Manvel Manvel force-pushed the chrome-privacy-api-update branch 2 times, most recently from 66869a3 to b082082 Compare Oct 14, 2019
},
};
// Wrap all "network", "services", "websites" APIs in chrome.privacy.*
if (extensionAPIs.privacy) {
Copy link
Member

@Rob--W Rob--W Oct 29, 2019

Choose a reason for hiding this comment

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

This dereferences the privacy namespace even when not used.

Could you make this a lazy getter? Maybe a getter for each of network, services and websites.

Copy link
Contributor Author

@Manvel Manvel Oct 29, 2019

Choose a reason for hiding this comment

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

Good point, will update.

Copy link
Contributor Author

@Manvel Manvel Oct 29, 2019

Choose a reason for hiding this comment

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

Updated, I have created separated getters for each of the privacyTypes as you have asked, but I feel like that implementation below is simpler:

// Wrap all "network", "services", "websites" APIs in chrome.privacy.*
    const privacy = {
      get supported() {
        return extensionAPIs.privacy;
      },
    };
    if (privacy.supported) {
      apiMetadata.privacy = {};
      for (const privacyType of ["network", "services", "websites"]) {
        apiMetadata.privacy[privacyType] = {};
        for (const privacyName of Object.keys(extensionAPIs.privacy[privacyType])) {
          apiMetadata.privacy[privacyType][privacyName] = settingMetadata;
        }
      }
    }

What you think?

apiMetadata.privacy = {};
for (const privacyType of ["network", "services", "websites"]) {
apiMetadata.privacy[privacyType] = {};
for (const privacyName in extensionAPIs.privacy[privacyType]) {
Copy link
Member

@Rob--W Rob--W Oct 29, 2019

Choose a reason for hiding this comment

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

This would also include enumerable properties from Object.prototype, which shouldn't be exported.

Copy link
Contributor Author

@Manvel Manvel Oct 29, 2019

Choose a reason for hiding this comment

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

Good point, will update.
Guess using Object.keys() might be better.

Copy link
Contributor Author

@Manvel Manvel Oct 29, 2019

Choose a reason for hiding this comment

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

Done.

@Manvel
Copy link
Contributor Author

Manvel commented Oct 29, 2019

@Rob--W I have addressed the comments, but seems like there is some problems with ChromeDriver SessionNotCreatedError: session not created: This version of ChromeDriver only supports Chrome version 76.

Most probably I won't be available tomorrow, but will ensure to address anything needed the day after.

Copy link
Member

@Rob--W Rob--W left a comment

@Rob--W I have addressed the comments, but seems like there is some problems with ChromeDriver SessionNotCreatedError: session not created: This version of ChromeDriver only supports Chrome version 76.

Most probably I won't be available tomorrow, but will ensure to address anything needed the day after.

We should probably bump the chromedriver version again... As done in #199. If you open a pull request in the same format, I'd approve and merge it.

return {};
}});
}
if (privacy.supported) {
Copy link
Member

@Rob--W Rob--W Oct 29, 2019

Choose a reason for hiding this comment

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

This is not a lazy getter.

As soon as you dereference the extensionAPIs.privacy object, the browser has to initialize the extension bindings for chrome.privacy object.

A lazy getter is one where you avoid dereferencing the extensionAPIs.privacy object until absolutely necessary.

For example, something like this:

apiMetadata.privacy = {
  get network() { // <-- This is a getter. You should probably use Object.defineProperty instead.
    let target = extensionAPIs.privacy && extensionAPIs.privacy.network;
    if (!target) {
      let network = {};
      for (let k of Object.keys(target)) {
       network[k] = settingMetadata;
      }
      this.network = network; // <-- This caches the result, hence the "lazy getter" is run only once.
      return network;
    }
  },
};

Copy link
Contributor Author

@Manvel Manvel Oct 29, 2019

Choose a reason for hiding this comment

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

Now I feel so stupid, thanks will update.

Copy link
Contributor Author

@Manvel Manvel Oct 29, 2019

Choose a reason for hiding this comment

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

@Rob--W Updated, hope now it looks better.

@Manvel
Copy link
Contributor Author

Manvel commented Oct 29, 2019

@Rob--W I have addressed the comments, but seems like there is some problems with ChromeDriver SessionNotCreatedError: session not created: This version of ChromeDriver only supports Chrome version 76.
Most probably I won't be available tomorrow, but will ensure to address anything needed the day after.

We should probably bump the chromedriver version again... As done in #199. If you open a pull request in the same format, I'd approve and merge it.

Done -> #208


apiMetadata.privacy = {};
for (const type of ["network", "services", "websites"]) {
Object.defineProperty(apiMetadata.privacy, type, {
Copy link
Member

@Rob--W Rob--W Oct 29, 2019

Choose a reason for hiding this comment

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

Add configurable: true, to the properties of the parameter to Object.defineProperty, to ensure that the property can be replaced later.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty

Copy link
Contributor Author

@Manvel Manvel Oct 30, 2019

Choose a reason for hiding this comment

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

done.

return privacyType;
}
},
set: (value) => value,
Copy link
Member

@Rob--W Rob--W Oct 29, 2019

Choose a reason for hiding this comment

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

This is only used internally; no setter necessary.

Copy link
Contributor Author

@Manvel Manvel Oct 30, 2019

Choose a reason for hiding this comment

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

When I don't use setters, getters only error is thrown when caching the value as the strict mode is used.

Not sure why this is not being caught by tests.

Steps to reproduce:

  1. npm run build.
  2. Load dist/browser-polyfill.js into a chrome extension.
  3. execute browser.privacy.services.passwordSavingEnabled.get({});

Observer behavior:

Uncaught TypeError: Cannot set property services of #<Object> which has only a getter

Expected:
No error is thrown.

Copy link
Member

@Rob--W Rob--W Oct 30, 2019

Choose a reason for hiding this comment

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

This is because you're triggering the setter (at line 505).

There are two ways to replace a getter property:

  1. delete the original property and assign the value.
  2. Use Object.defineProperty` to replace the property descriptor with one that only has a value.

In a unit test, you should count the number of accesses to chrome.privacy.network when you dereference the chrome.privacy.network object.

After looking at the implementation, it is apparent that the metadata is dereferenced only once and cached afterwards. I guess that you don't even have to replace the getter, so the code can be simplified.

implementation where metadata is dereferenced once:

(hasOwnProperty(wrappers, prop) ||
hasOwnProperty(metadata, prop))) {
// This is an object that we need to do some wrapping for the children
// of. Create a sub-object wrapper for it with the appropriate child
// metadata.
value = wrapObject(value, wrappers[prop], metadata[prop]);

Copy link
Contributor Author

@Manvel Manvel Oct 30, 2019

Choose a reason for hiding this comment

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

In a unit test, you should count the number of accesses to chrome.privacy.network when you dereference the chrome.privacy.network object.

I don't understand this comment, you want me to update the tests in a way?

After looking at the implementation, it is apparent that the metadata is dereferenced only once and cached afterwards. I guess that you don't even have to replace the getter, so the code can be simplified

Sounds good, then I guess I can just remove the line where I overwrite the getter and that's it?

Copy link
Contributor Author

@Manvel Manvel Oct 30, 2019

Choose a reason for hiding this comment

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

After looking at the implementation, it is apparent that the metadata is dereferenced only once and cached afterwards. I guess that you don't even have to replace the getter, so the code can be simplified

Done, removed the cache. Hope I understood your comment correctly.

In a unit test, you should count the number of accesses to chrome.privacy.network when you dereference the chrome.privacy.network object.

I need a hint here, not sure how I can implement that.

Copy link
Member

@Rob--W Rob--W Oct 30, 2019

Choose a reason for hiding this comment

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

Here is an existing test that verifies that a property is accessed only once:

describe("without side effects", () => {
it("should proxy non-wrapped methods", () => {
let lazyInitCount = 0;
const fakeChrome = {
get runtime() {
// Chrome lazily initializes API objects by replacing the getter with
// the value. The initialization is only allowed to occur once,
// after that `undefined` is returned and a warning is printed.
// https://chromium.googlesource.com/chromium/src/+/4d6b3a067994ce6dcf0ed9a9efd566c083736952/extensions/renderer/module_system.cc#414
//
// The polyfill should invoke the getter only once (on the global chrome object).
++lazyInitCount;
const onMessage = {
addListener(listener) {
equal(this, onMessage, "onMessage.addListener should be called on the original chrome.onMessage object");
},
};
const value = {onMessage};
Object.defineProperty(this, "runtime", {value});
return value;
},
get tabs() {
ok(false, "chrome.tabs should not lazily be initialized without explicit API call");
},
};
return setupTestDOMWindow(fakeChrome).then(window => {
// This used to be equal(lazyInitCount, 0, ...), but was changed to
// accomodate a change in the implementation of the polyfill.
// To verify that APIs are not unnecessarily initialized, the fakeChrome
// object has a "tabs" getter that fails the test upon access.
equal(lazyInitCount, 1, "chrome.runtime should be initialized because chrome.runtime.id is accessed during polyfill initialization");
window.browser.runtime.onMessage.addListener(() => {});
equal(lazyInitCount, 1, "chrome.runtime should be initialized upon accessing browser.runtime");
window.browser.runtime.onMessage.addListener(() => {});
equal(lazyInitCount, 1, "chrome.runtime should be re-used upon accessing browser.runtime");
window.chrome.runtime.onMessage.addListener(() => {});
equal(lazyInitCount, 1, "chrome.runtime should be re-used upon accessing chrome.runtime");
});
});
});

You would have to add a new test that verifies that fakeChrome.privacy.network is accessed only once, even if the test gets window.browser.privacy.network.somePropertyName twice.
It would probably also be good to verify that if fakeChrome.privacy.services is undefined, that window.privacy.network.services is undefined as well.

Copy link
Contributor Author

@Manvel Manvel Oct 30, 2019

Choose a reason for hiding this comment

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

@Rob--W I have added a test, looking into existing implementation you provided, but I'm bit unsure about it.

It would probably also be good to verify that if fakeChrome.privacy.services is undefined, that window.privacy.network.services is undefined as well

Not sure if I understood this one, are you referring to something like?

equal(window.browser.privacy.services, undefined);

@Manvel Manvel force-pushed the chrome-privacy-api-update branch from e735f85 to 8277c03 Compare Oct 30, 2019
@@ -178,6 +189,12 @@ describe("browser-polyfill", () => {

window.chrome.runtime.onMessage.addListener(() => {});
equal(lazyInitCount, 1, "chrome.runtime should be re-used upon accessing chrome.runtime");

window.browser.privacy.network.networkPredictionEnabled();
Copy link
Member

@Rob--W Rob--W Oct 30, 2019

Choose a reason for hiding this comment

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

Don't call the function, just check that the returned object looks like a Setting object (object with set, get and clear methods).

Copy link
Contributor Author

@Manvel Manvel Oct 30, 2019

Choose a reason for hiding this comment

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

Done.


const networkPredictionEnabled = () => true;
const value = {networkPredictionEnabled};
Object.defineProperty(fakeChrome.privacy, "network", {value});
Copy link
Member

@Rob--W Rob--W Oct 30, 2019

Choose a reason for hiding this comment

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

Don't replace the property. Now we're interested in how often privacy.network is dereferenced.

Copy link
Contributor Author

@Manvel Manvel Oct 30, 2019

Choose a reason for hiding this comment

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

Noted, done.

It fails, but I can confirm it's not a regression as it was calling twice for the old implementation as well.

Any idea why it's so? Might be a problem with my test?

Copy link
Contributor Author

@Manvel Manvel Oct 31, 2019

Choose a reason for hiding this comment

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

It fails, but I can confirm it's not a regression as it was calling twice for the old implementation as well.

This was wrong assumption, apparently I was running test without running npm run build first, Sorry for this, will have a look into it later today, the old tests were passing correctly, the problem is in the new implementation.

Copy link
Contributor Author

@Manvel Manvel Oct 31, 2019

Choose a reason for hiding this comment

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

After taking a closer look into implementation, now I see what is the problem.

The problem is that I can't fix it without touching wrapAPIs() code, unless I hardcode everything.

I'm thinking about something like:

const settingMetadata = {
      clear: {minArgs: 1, maxArgs: 1},
      get: {minArgs: 1, maxArgs: 1},
      set: {minArgs: 1, maxArgs: 1},
};

apiMetadata.privacy = {"*": {"*": settingMetadata}};

and updating wrapObject():

if (hasOwnProperty(metadata, "*")) {
 value = wrapObject(value, wrappers[prop], metadata["*"]);
}

I'm quite uncomfortable updating wrapObject(), but I'll give it a shot and revert it if you say so.

Copy link
Contributor Author

@Manvel Manvel Oct 31, 2019

Choose a reason for hiding this comment

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

@Rob--W Done.

@@ -140,6 +140,7 @@ describe("browser-polyfill", () => {
describe("without side effects", () => {
it("should proxy non-wrapped methods", () => {
let lazyInitCount = 0;
let lazyInitPrivacyCount = 0;
Copy link
Member

@Rob--W Rob--W Oct 30, 2019

Choose a reason for hiding this comment

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

Could you create a new describe(...) block to test the privacy API?

Copy link
Contributor Author

@Manvel Manvel Oct 30, 2019

Choose a reason for hiding this comment

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

Done.

referrersEnabled: settingMetadata,
},
};
apiMetadata.privacy = {"*": {"*": settingMetadata}};
Copy link
Member

@Rob--W Rob--W Nov 2, 2019

Choose a reason for hiding this comment

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

I'm not immediately against this implementation, since it is still somewhat readable and concise.
I do wonder whether we should explicitly spell out network, services, and websites.

@rpl Thoughts?

Copy link
Contributor Author

@Manvel Manvel Nov 7, 2019

Choose a reason for hiding this comment

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

@Rob--W Any thought on this? Happy to explicitly spell out network, services, and websites, if that holds the progress.

CC: @rpl

Copy link
Member

@rpl rpl Nov 7, 2019

Choose a reason for hiding this comment

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

👍 from me on spelling out the first level and use the "*" to cover all the properties inside those.

Copy link
Contributor Author

@Manvel Manvel Nov 7, 2019

Choose a reason for hiding this comment

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

Thanks @rpl and @Rob--W, done.

I was thinking about 2 different implementation:

const privacyType = {"*": settingMetadata};
apiMetadata.privacy = {
      network: privacyType,
      services: privacyType,
      websites: privacyType,
};
apiMetadata.privacy = {};
for (const type of ["network", "services", "websites"]) {
  apiMetadata.privacy[type] = {"*": settingMetadata};
}

If you have preference for one or another please let me know which to use. This seem to be most consistent comparing to settingMetadata.

Copy link
Member

@Rob--W Rob--W Nov 7, 2019

Choose a reason for hiding this comment

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

Keep the current code. It is more readable than the proposed alternatives.

},
};

return setupTestDOMWindow(fakeChrome).then(window => {
Copy link
Member

@Rob--W Rob--W Nov 7, 2019

Choose a reason for hiding this comment

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

equal(lazyInitCount, 0, ...); to check that chrome.privacy is not accessed at first.

Copy link
Contributor Author

@Manvel Manvel Nov 7, 2019

Choose a reason for hiding this comment

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

Done.

Rob--W
Rob--W approved these changes Nov 7, 2019
@Manvel
Copy link
Contributor Author

Manvel commented Nov 14, 2019

@Rob--W Is there anything blocking this issue to be merged into master?

Thanks in advance.

@Rob--W Rob--W merged commit 87bdfa8 into mozilla:master Nov 21, 2019
1 check passed
@Rob--W
Copy link
Member

Rob--W commented Nov 21, 2019

Thanks for the patch @Manvel !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants