(in e10s window) Greasemonkey doesn't intercept some userscript links in GitHub (by presenting the installation dialog), though they end with 'user.js' #2280

Closed
darkred opened this Issue Sep 18, 2015 · 22 comments

Projects

None yet

3 participants

@darkred
darkred commented Sep 18, 2015

(using FF Nightly 43.0a1 x64, with GM 3.4.1, in a e10s window)

For example:
the link that is the Raw button in this page

The URL is:
https://github.com/jerone/UserScripts/raw/63b0f7fd9818c780d05635c9f5acb26afe2d05ef/Github_News_Feed_Filter/Github_News_Feed_Filter.user.js
The same link, linkified here is intercepted ok.

On the other hand, in a non e10s window (and in FF40 w GM 3.4.1) the link is intercepted ok.


For reference, for that same script (in a e10s window), the installation gets intercepted ok
if you either press the Install button in the script's main page(master branch)
(or if you press the Github_News_Feed_Filter.user.js link and then the Raw button in that link).

@darkred darkred closed this Sep 18, 2015
@darkred
darkred commented Sep 18, 2015

I just reopened it (closed by mistake)

@darkred darkred reopened this Sep 18, 2015
@janekptacijarabaci
Contributor

See: https://github.com/greasemonkey/greasemonkey/blob/3.4.1/content/browser.js#L34

e10s on: The listener will not start.

  • gBrowser runs in the chrome/parent process, so it will see events for chrome pages, but not normal web content

e10s off: The listener starts.

@arantius arantius added this to the 3.5 milestone Sep 23, 2015
@arantius arantius modified the milestone: 3.6, 3.5 Sep 23, 2015
@arantius
Collaborator

I've been having trouble understanding/confirming this issue. I've merged @janekptacijarabaci 's proposed fix and would appreciate feedback on whether 3.5beta3 helps:

https://addons.mozilla.org/firefox/downloads/file/356688/greasemonkey-3.5beta3-fx.xpi?src=devhub

@arantius
Collaborator

In the originally reported case of URL:

https://github.com/jerone/UserScripts/blob/master/Github_News_Feed_Filter/Github_News_Feed_Filter.user.js

The URL already ends in .user.js but it's an HTML page describing the source control repository for that file, so we display it rather than intercepting it for script install. Following the link from there to another URL ending .user.js at this point intentionally does not install due to the fix for #1875.

@arantius arantius modified the milestone: 3.6, 3.5 Oct 14, 2015
@arantius arantius modified the milestone: 3.7, 3.6 Nov 20, 2015
@arantius
Collaborator
arantius commented Feb 3, 2016

Fixing this bug has to include not breaking #1875. Refreshing myself on the detail of that one.

Trying to edit a .user.js file purely via GitHub web UI goes like:

  1. GET to https://github.com/USER/REPO/edit/master/FILE.user.js
  2. click 'commit changes'
  3. POST to https://github.com/USER/REPO/tree-save/master/FILE.user.js

The code committed to fix #1875 implies that we were intercepting the POST in step 3, REJECTing it because it looks like a user script URL, and then that breaks the POST data. The change was to detect that seemingly unique case and instead ACCEPT the request that was probably a POST (though we don't have any context to know for certain that it is a POST).

@arantius
Collaborator
arantius commented Feb 3, 2016

I've created a test script to reproduce this issue, without using public GitHub: https://gist.github.com/arantius/b57b7c0fc467237d0062

And yes, I've confirmed the situation back from #1875 -- the solution (really, workaround) to that is the cause of this. The proper fix for this is to fix #1875 without causing this side effect.

@arantius
Collaborator
arantius commented Feb 3, 2016

My best idea so far, but I don't like how big a change it would be, is to switch from nsIContentPolicy to http-on-modify-request. The latter should be able to do a .cancel() on the request (to switch from loading the script to installing it), but would be a lot of new potentially buggy code to add.

@arantius
Collaborator
arantius commented Feb 3, 2016

I've started working on the approach described immediately above. A few false starts were frustrating, but now I've made some good/interesting progress.

@arantius
Collaborator
arantius commented Feb 3, 2016

Whoops, the above was e10s disabled. Enabled, giant mess. Plus http-on-modify doesn't see local files.

@arantius
Collaborator

Some snippets for my own records:

1

var runtime = Cc["@mozilla.org/xre/app-info;1"]
    .getService(Ci.nsIXULRuntime);
dump('Running in parent? '
    + (runtime.processType === Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT)
    + '\n');

2

var browser = httpChannel
    .notificationCallbacks.getInterface(Ci.nsILoadContext)
    .topFrameElement;

Which are "useful" but don't actually do what I need in context of the work in progress so far ( arantius@master...install-observer ) for this issue.

@arantius arantius modified the milestone: 3.8, 3.7 Feb 11, 2016
@arantius
Collaborator

The core of the problem I'm facing: http://stackoverflow.com/questions/35344579/

@arantius
Collaborator

Ah! I've rearranged the problem into something that seems workable. The http-on-modify-request observer is documented (kinda) to only work in the parent, so it has to live there. But I don't truly need to send a message to the client, I just need to control whether the request proceeds or not.

And the critical new bit: I can not only .cancel(), but also .suspend() and .resume() the nsIRequest! So rather than .sendAsyncMessage() to get the child to load the page (after we start to download it for an install, and discover that it's an HTML page, not a script), just .resume() it and it works like we never did anything.

That and restoring(/keeping) the nsIContentPolicy to see local files (HTTP observer only sees HTTP) is looking like it works. Removes some janky existing code, but needs to be tested, definitely. Especially the less common features and edge cases around installs.

@arantius
Collaborator

Thanks, that was unintentional, one of the many things I tried and removed, failed to clean up.

0d023e0

@arantius
Collaborator

I'm just noticing now that even though .cancel() is getting called, the spinner in the tab continues to spin forever.

@arantius arantius added a commit to arantius/greasemonkey that referenced this issue Feb 19, 2016
@arantius arantius Rewrite script install manager.
Switch from nsIContentPolicy to http-on-modify-request observer.  This lets us properly detect a POST, for a proper fix to #1875, so we can remove the suboptimal workaround solution to that.

Fixes #2280
fe737c7
@arantius arantius added a commit that closed this issue Feb 19, 2016
@arantius arantius Rewrite script install manager.
Switch from nsIContentPolicy to http-on-modify-request observer.  This lets us properly detect a POST, for a proper fix to #1875, so we can remove the suboptimal workaround solution to that.

Fixes #2280
fe737c7
@arantius arantius closed this in fe737c7 Feb 19, 2016
@arantius
Collaborator

I've just pushed what should be a fix for this, but it's a big change. I would greatly appreciate any help testing that it works comprehensively; build 3.8beta1 includes this change:

https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/versions/?page=1#version-3.8beta1

@janekptacijarabaci
Contributor

@arantius

  1. Ad fe737c7#diff-97a81218d24e6e5e19091540aefc4331R10
    Twice the same line: Cu.import("resource://gre/modules/Services.jsm");

  2. Ad #2280 (comment):
    An issue is returned:
    #2292
    (update: #2292 (comment))
    It still holds true...

@arantius arantius reopened this Feb 20, 2016
@arantius
Collaborator

@janekptacijarabaci Please please use more words. I don't know what you mean at all for point 2.

@janekptacijarabaci
Contributor

Tested on:
GM 3.8beta1+
FF e10s: off

This bug is back ("[e10s off] Two install dialogs"):
#2292

But only version 44.0.2-. The issue has been fixed in newer versions of Firefox (45.0beta1+).

I'm sorry.


Last bad: 2016-02-01
Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0
https://hg.mozilla.org/mozilla-central/rev/941033a51983ddec2d99aa9f868a54c0196a4075

First good: 2016-02-02
Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0
https://hg.mozilla.org/mozilla-central/rev/5f9ba76eb3b1fd9377bbdb4cc2f98a7e75eabdfb

Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=941033a51983ddec2d99aa9f868a54c0196a4075&tochange=5f9ba76eb3b1fd9377bbdb4cc2f98a7e75eabdfb

@arantius
Collaborator
arantius commented Mar 4, 2016

But only version 44.0.2-. The issue has been fixed in newer versions of Firefox (45.0beta1+).

That's probably why I didn't catch it. And Firefox 45 will be stable soon, so I think I'm comfortable living with this minor usability issue until it is.

@arantius arantius closed this Mar 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment