avoid sync messages on document load #2259

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@the8472
Contributor
the8472 commented Aug 26, 2015

This removes the sync greasemonkey:scripts-for-url message from the framescript.

builds on work from PR #2243 and #2255

the8472 added some commits Aug 18, 2015
@the8472 the8472 avoid holding duplicate script source strings in memory
old: strings sent via e10s message manager are duplicated and the
sandbox holds onto them as source code, this causes unnecessary memory
overhead.

new: load scripts from file:// URIs and use lazy getters for
scriptSource
70a16c4
@the8472 the8472 Support GM_xmlhttpRequest referer override in e10s e4a38a3
@janekptacijarabaci
Contributor

The warning:

Services.ppmm

(Services.mm, Services.cpmm)

Not compatible for Firefox 38.0.5-
https://hg.mozilla.org/mozilla-central/diff/1b3657fee5a8/toolkit/modules/Services.jsm

@arantius
Collaborator
  1. Thanks for all your effort @the8472
  2. Please read http://greasemonkey.github.io/style.html and follow it in your pull requests . (I see lots of missing spaces and braces, and the general style is to prefix function arguments' names with an "a".)
  3. I'd find it much easier to review/understand pull requests if they were independent; this seems to be a mix of some other ones, plus further changes.

What's the overall approach here? Effectively the whole config of all scripts and their include/exclude data is stored in every child all the time? And the fileXhr trick means that we don't have to duplicate the script itself?

@arantius arantius added this to the 3.5 milestone Aug 26, 2015
@the8472
Contributor
the8472 commented Aug 26, 2015

The Warning

ok, will fix that

style

Is there some auto-formatter I could apply? That's more a job for a machine than a human.

pull requests review

Once you looked at the other two you only need to review f21e40a

Effectively the whole config of all scripts and their include/exclude data is stored in every child all the time? And the fileXhr trick means that we don't have to duplicate the script itself?

Yes. In each child process, not in each frame.

Additionally, the old code used the sync messages to check for script refreshes, this now piggybacks on http observers which carry similar information to the sync messages anyway. We can extract all the necessary information from requests with TYPE_DOCUMENT/TYPE_SUBDOCUMENT in their loadInfo.

@janekptacijarabaci
Contributor

The warning:

initialProcessData

Not compatible for Firefox 42- (maybe 41- , https://bugzilla.mozilla.org/show_bug.cgi?id=1162838 , but: 41.0b6 - in this version does not work).

@janekptacijarabaci janekptacijarabaci and 1 other commented on an outdated diff Sep 13, 2015
modules/ipcscript.js
this.localized = aScript.localized;
this.matches = aScript.matches.map(function(m) { return m.pattern; });
+ this.userMatches = aScript.matches.map(function(m) { return m.pattern; });
@janekptacijarabaci
janekptacijarabaci Sep 13, 2015 Contributor

I guess it should be: matches => userMatches

@the8472
the8472 Sep 13, 2015 Contributor

yes, thanks

@janekptacijarabaci janekptacijarabaci commented on the diff Sep 13, 2015
modules/ipcscript.js
@@ -60,8 +123,6 @@ IPCScript.prototype.info = function() {
return {
'uuid': this.uuid,
'version': this.addonVersion,
@janekptacijarabaci
janekptacijarabaci Sep 13, 2015 Contributor

I think it will be undefined...

@the8472
the8472 Sep 13, 2015 Contributor

what is "it"?

@janekptacijarabaci
janekptacijarabaci Sep 13, 2015 Contributor

this.addonVersion => GM_info.version
I think it will be undefined...

@the8472
the8472 Sep 13, 2015 Contributor

I did not change this code. this.addonVersion is set in the constructor

@janekptacijarabaci
janekptacijarabaci Sep 13, 2015 Contributor

Yes, but:
https://github.com/greasemonkey/greasemonkey/pull/2259/files#diff-e07419236596eaa414a5954c7c90c385R151

However, this does not help:
return new IPCScript(script, gGreasemonkeyVersion);

GM_info.version = "unknown";

@the8472
the8472 Sep 13, 2015 Contributor

Ok, thanks for digging, i'll have a look at it.

@janekptacijarabaci janekptacijarabaci commented on an outdated diff Sep 13, 2015
modules/sandbox.js
+ content = GM_util.fileXHR(scriptURL, "application/javascript");
+ fileCache.set("scriptSource", content);
+ }
+ return content;
+ }, sandbox)
+ }
+ );
+
+ // meta depends on content, so we need a lazy one here too
+ Object.defineProperty(waivedInfo,
+ 'scriptMetaStr',
+ {get: Cu.exportFunction(
+ function() {
+ var meta = fileCache.get("meta");
+ if(meta === undefined) {
+ meta = extractMeta(this.scriptSource);
@janekptacijarabaci
janekptacijarabaci Sep 13, 2015 Contributor

It throws the error:
1

@the8472
Contributor
the8472 commented Sep 13, 2015

@janekptacijarabaci thanks for testing, I think those issues should be fixed now.

@janekptacijarabaci
Contributor

You're welcome.

However, I see another problem...

After editing the script in Scratchpad (e.g.) and saving - properties of the object (GM_info) is not valid (is not changed):
https://github.com/greasemonkey/greasemonkey/pull/2259/files#diff-9f06c50d889eb78e6776c4b4fc7314cbL66

@the8472
Contributor
the8472 commented Sep 14, 2015

@janekptacijarabaci when I save + refresh the page GM_info.script does update accordingly for me

demo: https://i.pantsu.cat/lemhtw.webm

It's pretty much the same old logic, just that it infers a page load from a HTTP observer instead of relying on sync messages from the frame script: https://github.com/the8472/greasemonkey/blob/f21e40a74147e409231e39b08f9387e55cc0cb07/modules/refererSetter.js#L33-L53

@janekptacijarabaci
Contributor

I'm sorry, but I do not work for me.
Firefox Nightly 43.0a1 (2015-09-14)
Firefox 40.0.3

Steps to reproduce:

  1. I trying make a new profile of Firefox (but in the default profile too)
  2. Installing Greasemonkey (your version)
  3. Installation of this script: https://gist.github.com/janekptacijarabaci/e4a939351e5792d3ba90
  4. Open Browser Console
  5. Open page github.com in browser - only one page is open (one tab)
    5a. In Browser Console: e.g. GM_info.script.version: 1.0
  6. Open Scratchpad, the script editing (@version 1.0 => @version 1.1), saving
  7. Refresh github.com in browser
    7a. In Browser Console: GM_info.script.version: 1.0

I don't have time to find the bug now...

@janekptacijarabaci
Contributor

when = "any": this value does not exist...

Fix this bug (for example):

From:

GM_util.getService() 
    .scriptRefresh("any", channel.URI.spec, windowId, browser);

To:

GM_util.getService() 
    .scriptRefresh("document-start", channel.URI.spec, windowId, browser);
GM_util.getService() 
    .scriptRefresh("document-end", channel.URI.spec, windowId, browser);

...or:

From:

.scriptRefresh("any", channel.URI.spec, windowId, browser);

To:

.scriptRefresh(channel.URI.spec, windowId, browser);

From:

service.prototype.scriptRefresh = function(when, url, windowId, browser) {

To:

service.prototype.scriptRefresh = function(url, windowId, browser) {

From:

this.config.updateModifiedScripts(when, url, windowId, browser);

To:

this.config.updateModifiedScripts("document-start", url, windowId, browser);
this.config.updateModifiedScripts("document-end", url, windowId, browser);
the8472 added some commits Sep 15, 2015
@the8472 the8472 Merge branches 'e10s-referer' and 'dedup_string' into async-messages2 0bfa0ef
@the8472 @the8472 the8472 reduce sync messages from frame scripts
- beam down IPCScripts in advance from parent when a script changes
- do script filtering/url matching in content process
- piggyback script change detection on http observers instead by
checking document/sub-document loads
70fab3e
@the8472
Contributor
the8472 commented Sep 15, 2015

ok, incorporated your changes and rebased on master

@arantius
Collaborator

I've started merging this down. It's a huge change, I want to give it some time to bake before I commit it to master.

master...arantius:broadcast-script-config

@arantius
Collaborator

Point one: no, intentional, merge conflict with c274845 .

Point two: good catch!

@arantius
Collaborator

See #2289 -- doesn't work with e10s off. Looking into why now.

@arantius
Collaborator

Also not working in FF 44 with e10s off, so e10s off appears to be the real issue.

@the8472
Contributor
the8472 commented Sep 24, 2015

Odd, I did test with e10s off, maybe I skipped that during the recent rebase.

Anyway, how do you want fixes? New commits on this PR? Rebase? Or against your own branch?

@arantius
Collaborator

Gimme 5, I might be near a "fix". Something at appears to work better, assuming I understand the operating model.

@arantius arantius added a commit to arantius/greasemonkey that referenced this pull request Sep 24, 2015
@arantius arantius Service broadcastScriptUpdates(): set initial AND broadcast.
Refs #2259
73736ed
@arantius
Collaborator

I think 73736ed is the right fix for #2289 and some related issues.

@the8472
Contributor
the8472 commented Sep 24, 2015

Hum, was that some merge conflict? The broadcast shouldn't have been in an else block. It doesn't exist in my branch

@arantius
Collaborator

Indeed, must have been my fault somehow. Whoops, there it is.

@the8472
Contributor
the8472 commented Sep 24, 2015

In case you missed this file "modules/refererSetter.js"
Point one: no, intentional, merge conflict with c274845 .

What about the script refresh part?

@arantius
Collaborator

I didn't know it existed because it was in a file called refererSetter.js. Add it back in a place that makes a bit more sense please?

@janekptacijarabaci
Contributor

The reminder (a typo):
#2259 (comment)

@arantius arantius added a commit to arantius/greasemonkey that referenced this pull request Sep 25, 2015
@arantius arantius Fix filename reference typo.
Refs #2259
1c58833
@janekptacijarabaci
Contributor

The suggestion:
arantius#3

@arantius
Collaborator

Merged this into master.

@arantius arantius closed this Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment