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

Initial work on upgrading OpenWPM to Firefox ESR 60 #221

Merged
merged 22 commits into from
Oct 23, 2018

Conversation

motin
Copy link
Contributor

@motin motin commented Sep 29, 2018

Work in progress, not ready for merge yet. Created for discussion and ongoing review purposes.

Relates to #148

@englehardt In order to make this a collaborative effort, please create a firefox-upgrade or develop branch in the OpenWPM repo for this PR to target, so that the master branch can still be used for bugfixes to the stable version while we get the major version upgrade in order.

(Note: this PR includes the commits from #219 as well. It is advised to first review and merge that PR, whereafter this PR will be rebased on the current master branch, making it easier to review)
EDIT: #219 has been merged and this PR has been rebased.

@englehardt englehardt mentioned this pull request Oct 1, 2018
@motin motin force-pushed the fx60-esr-init branch 3 times, most recently from 8ca5f73 to 133135f Compare October 2, 2018 09:05
@motin
Copy link
Contributor Author

motin commented Oct 2, 2018

(Note that travis is not be able to install the proper selenium package due to SeleniumHQ/selenium#6470 - thus all travis tests fail until the upstream issue is fixed or a workaround is implemented)

@motin motin changed the base branch from master to develop October 2, 2018 19:04
@motin motin force-pushed the fx60-esr-init branch 3 times, most recently from 3e79ad0 to aaff4ed Compare October 2, 2018 22:21
@motin
Copy link
Contributor Author

motin commented Oct 2, 2018

This is ready to merge into the develop branch.

@motin motin force-pushed the fx60-esr-init branch 2 times, most recently from 6561d52 to bbcd0d8 Compare October 2, 2018 22:51
motin added a commit to motin/openwpm-webext-instrumentation that referenced this pull request Oct 2, 2018
motin added a commit to mozilla/openwpm-webext-instrumentation that referenced this pull request Oct 2, 2018
Fix Dockerfile-dev build, by ADDing requirements-dev.txt to /opt/Open…
Copy link
Collaborator

@englehardt englehardt left a comment

Choose a reason for hiding this comment

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

@motin Once again, great work!

"content.js"
],
"run_at": "document_start",
"all_frames": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also add "match_about_blank": true, (see: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/content_scripts). Note that about:blank iframes can execute JS as they are same-origin to the embedding frame (https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy#Inherited_origins).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point


// Wait for window.instrumentObject to be set by
// the injected page script before continuing
const instrumentObjectAvailable = new Promise(function(resolve) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know we discussed this briefly on our call, but I don't remember the conclusion. Can you reiterate why we experience a delay, i.e. why injecting at document_start isn't sufficient? This will cause us to miss JS calls that happen very early during a page visit.

Copy link
Contributor Author

@motin motin Oct 23, 2018

Choose a reason for hiding this comment

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

Currently, there is no way for webextensions to inject page scripts before any ordinary web content is loaded, so there is always a race condition between the ordinary web content and the injected page script, hence this wait for the instrumentation to be available before attempting to use it. Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1378459

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, if I'm understanding Bug 1378459 correctly, the issue is related to extension startup and thus will be present on the first page load only right? I suspect we can have the browser manager process block on a signal from the extension before submitting the first top-level request. I'll file a new bug for this.

@motin
Copy link
Contributor Author

motin commented Oct 23, 2018

Rebased and ready for merge. Also including the latest commits to master, so that the develop branch is up to date.

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.

3 participants