gorhill / uMatrix

uMatrix: Point and click matrix to filter net requests according to source, destination and type
GNU General Public License v3.0
4.55k stars 470 forks source link

uMatrix icon only available in one browser window when auto-restoring multi-window session #357

Closed DjogaRo closed 8 years ago

DjogaRo commented 9 years ago

Hi there, first, thanks for this add-on! When I learned about it, I even considered using Firefox again - well for a moment at least. Love that you made the efford to make it compatible with Pale Moon.

I'm using Pale Moon 25.7.0 (x64) with TabMixPlus on Win7 Pro SP1 64-Bit. Edit: uMatrix 0.9.2.1 TMP might be relevant, since I use its session manager. Edit/Update: It is irrelevant.

The problem manifests, when I start Pale Moon and a previous session containing multiple windows gets automatically restored. Only the first window gets to have the uMatrix icon. For the other windows not having the icon means, it's neither where I placed it (if I placed it) nor does it show up in the "Customize Toolbar" icon pool window.

When I open additional windows, those will have the icon. When I restored a 3-window session manually, 2 out of 3 windows did have the icon.

I use my setup for years now, my ongoing session having more then one window ain't anything new either, and I use several add-ons that come with buttons I placed in the toolbar, but for this behaviour I cannot give another example.

I hope there's a straight forward fix to this and coming soon, since TabMixPlus restores a sessions windows in weird order and I seldomly get the icon in the most important one.

Update: I just disabled TabMixPlus, closed all tabs and windows, restarted, set Pale Moon to restore last time tabs and windows on start, opened some windows with a few tabs (at that point all windows had the icon), exited PM, started it again, and experienced the same problem. So TabMixPlus seems not to be relevant.

After that I re-enabled TMP, restarted Pale Moon - only one window with pinned tabs at that moment -, restored my main session manually (not replacing the open window). I noticed that now it was reversed which window had the icon and which not. The already open window did have it, the first from the session that got opened did not, the second and third did, curious.

Update: Sidenote: uBlock Origin suffers the same problem, plus being a bit forgetfull regarding its settings. I've got a feeling the n/a-thing and the forgetfullness might share some common ground. The latter I will make an issue in the other project.

Pale Moons first start after booting Windows today was different. The last session (3 windows) got auto-restored as usual, but the problem didn't manifest at all. All 3 windows had the icons of uMatrix and uBlock_o on the addressbars right side, where I put them. _Also uBlocko had re-remembered its settings. I promptly exited PM and started over and both problems were back. That's why I tend to assume common ground. BTW, what I've reported before this 2nd update all happened with uBlock_o not being installed yet.

By now I'd say when PM starts for the 2nd, 3rd etc. time during a Windows session not all events get fired you rely upon for initialization. ? Or you register differently than other add-ons do? (just guessing; don't know the API)

Next I'll try with fresh, clean PM profiles, try it from a problem-free state closing a window and restoring it through History -> Recently closed windows ..., and maybe I'll install FF and try to reproduce the problem there. Till then ...

DjogaRo commented 8 years ago

Testing yielded two significant aspects: Pinned Tabs and restoring a window through History

Did some testing with a new Pale Moon profile and uMatrix only. I didn't change uMatrix' default settings. I changed PMs Home Page to "google.com/ncr", which resolves to "https://www.google.com/?gws_rd=ssl". I only used this page in every tab during testing. And I changed the PM start up setting to restore windows and tabs from last time.

Having 2 windows with 1 tab each, exiting and starting PM again yielded no problem, i.e. both windows kept the uMatrix icon. But the problem can be reproduced having 2 tabs in the first window with the first one being pinned. (Having 2 tabs in first window, none pinned -> no problem)

Then I tried to reproduce the problem when closing a window and restoring it through History -> Recently Closed Windows ... Strangely enough that procedure has the opposite effect. It doesn't produce the problem, it repairs it. So if I yielded the problem through exit PM, start PM, and then closed the window with the icon missing to restore it through history, the restored window got the icon. That "healing effect" even lasts through the next exit-start-PM iteration. So the latter has to be done 2 times to have a problem again.

Next thing on my list: doing the same with FF. Till then ...

DjogaRo commented 8 years ago

Tried the same testing with Firefox 40.0.3. Could not reproduce.

DjogaRo commented 8 years ago

Same test, Firefox 24.8.1esr Reproducible. Basically the same as with Pale Moon, but no need for pinned tabs, thus easier to reproduce. Although, with FF the problem was a bit fluctuating. First it was that after restoring a problematic window through History -> Recently Closed Windows ... it often took 3 instead of 2 iterations of exiting and starting FF to have an icon missing again. Then although uMatrix' default settings allow 1p cookies, when starting FF the restored tabs got redirected to google.de. And after that episode the history restore had lost its lasting effect, i.e. now it constantly took only one exit-start-FF to have a window missing the icon.

All very strange.

DjogaRo commented 8 years ago

For a few starts of PM now my problems with uMatrix and uBlock0 seem to be gone (including the mentioned forgetfulness). My current version of PM is 25.7.2 (x64), but i can't tell if the problems' end coincided with a PM-update. uMatrix still shows version number 0.9.2.1.

Though those problems don't bother me anymore, I confirmed that my last post is still valid. Well, except for the PM reference.

DjogaRo commented 8 years ago

Uhm, that might have been a bit premature. PM just got updated to 25.7.3 and I got both problems back. I don't know, if this is important, but i had icons missing again with the asked-for restart after update. Then I exited manually and started PM up again. Still, the problems are back (including uBlock0's forgetfulness).

gorhill commented 8 years ago

I did some work re. toolbar button in uBlock Origin (1.3.0rc4). If this resolves the toolbar button issues for uBlock Origin, I will just import these fixes in uMatrix.

DjogaRo commented 8 years ago

Thanks for your response. So I just made a new profile and installed uBlock0 from the link you provided and went through my test routine. Whatever you did definetly has an effect. Now when I cycle through (Main Menu -> "Exit", Start PM) the toolbar button is missing every second time. When I mess around like changing which window's Main Menu I use to exit, it sometimes needs a few iterations to fall back into this pattern. But apart from that the pattern is stable. So, not statistically every second time, but literally. Strange, but a change. :)

DjogaRo commented 8 years ago

Same test, Firefox 24.8.1esr Same same, but different. Here it's not that strict, alternating pattern. It seems to be a random series of the icon being there, not being there.

DjogaRo commented 8 years ago

Correction: Further playing around yielded the missing of the button in all but one window on auto-session-restore is random in PM, too. That also goes for the test profiles with uMatrix 0.9.2.1 or uBlock0 1.2.1 respectively. So I guess it has always been random for me and presented a pattern by chance or some things do have kind of an influence or changes to PM might have caused a pattern shift.

However, you indirectly led me to https://github.com/gorhill/uBlock/issues/833. And, you saying

I added another chunk of code to further defer adding the toolbar icon at a later time if uBlock can't find the required component when a new window is created.

made something else catch my eye.

old := in PM with uBlock0 1.2.1 new := in PM with uBlock0 1.3.0rc4

The old behaviour when starting PM, in every restored window - in which the button gets to be - said button starts out being empty bearing no picture yet and only having little extent, as if maybe caused by some set padding. Then the picture appears; the button is complete. The new behaviour is, the button starts out complete and then continue with the old behaviour from its beginning, i.e. being empty for a fracture of a second.

And that's actually all the difference I can make out so far. On opening new windows I cannot make out such thing happening (maybe just faster?). And I don't see it happening this way in FF24.

I also read the PM-Forum thread linked in uBlock issue 833. Others report to have the button missing in their only window and in new windows, which never was the case for me. The first restored window on PM start always has it. So does every new window I open.

In my standard profile with my standard session I seemingly always get the button in only the first window. But that's loaded with some more extensions plus the first window has 83 tabs - 27 of them pinned - and the second has another 20 tabs. Some of that might shift the odds.

DjogaRo commented 8 years ago

Update: The button placing code seems not to be a problem here. After I found out, that one can use unpacked add-ons and got the browser to show me some logging I put in, I observe that addLegacyToolbarButton(...) gets called as many times as I get windows bearing the button, i.e. Whenever I got the button missing in a window, it is because addLegacyToolbarButton(...) hasn't been executed for that window.

Is that the point where I change this issue's title? If so, to what?

gorhill commented 8 years ago

Whenever I got the button missing in a window, it is because addLegacyToolbarButton(...) hasn't been executed for that window.

Thanks, that helps narrow down where to look in the code. I will publish an interim build later just for the sake of checking whether the problem is fixed with further changes.

gorhill commented 8 years ago

rc6 on Releases page -- will check feedback later when I am back.

DjogaRo commented 8 years ago

Thanks. Did the testing and unfortunately could still reproduce. I'll pave that thing with console.log() statements and poke around later this day.

I haven't mentioned this here, yet, since I was going to make it a separat issue, but in the new context... When uBlock0 is missing its button, it also misses to use "My Filters". When I open the dashboard they are listed, but only after adding sth. and applying changes, the list takes effect again.

DjogaRo commented 8 years ago

Right now I'm at work, but for later it would be great to know the entry points already - for when their is a new browser instance and for when their is a new window. So, if you can provide them from the top of your head, it would help me a lot.

gorhill commented 8 years ago

Just to be sure... Are you testing using uBlock?

This is the entry point for when a new chrome window is created: https://github.com/gorhill/uBlock/blob/master/platform/firefox/vapi-background.js#L1159

DjogaRo commented 8 years ago

Yes|No, I'm testing using uBlock Origin to be exact. :) Both Buttons (uBlock0, uMatrix) are always present together or missing together. Well since there's an exception to everything, This morning in my second window (of two) uBlock's was there, uMatrix' Button was not. So, I presume, it doesn't matter which of them I use, does it.?

And thanks for the info. (still not back home)

gorhill commented 8 years ago

As said all the attempted fixes are in uBO, not in uMatrix, nothing has changed in uMatrix. If the fixes work for uBO, then I will import them to uMatrix. The point is to validate whether the fixes in uBO works in latest release.

DjogaRo commented 8 years ago

In case of a missing button onWindowLoad() did not get executed for that window. What next?

gorhill commented 8 years ago

You console.logged uBO, right?

If so, the only reason that I can think for this to happen is that Firefox is not passing the domwindowopened event to uBO. This event is received and handled at line 1219, in windowWatcher.observe. I don't see how this could be the case, hence I did not think it was useful to console.log this. Maybe you could try to just confirm the event is indeed really not fired, or if it is fired, see what is the value of topic?

DjogaRo commented 8 years ago

Correct, uBO 1.3.0.

vAPI.tabs.getWindows produces exactly one window, that would be why the first to be restored window always gets the button. But windowWatcher.observe does not always get executed for the remaining ones. (I put the logging at this function's very beginning.)

Would be interesting to see what happens, when first the windowWatcher gets registered and then the intial loop over existing windows gets done. I'll try that tomorrow. Just to rule out, that the remaining windows are produced between said loop and the registration.

DjogaRo commented 8 years ago

Just tried the latter. Makes no difference.

gorhill commented 8 years ago

vAPI.tabs.getWindows produces exactly one window

Hmm interesting. I see a test against a closed attribute at line 691, I wonder if this could be related...

DjogaRo commented 8 years ago

I made it test for (true || !win.closed), but still only one windows produced. In cases of error and cases of success (regarding button's presence). I really gotta get some sleep. Til then.

gorhill commented 8 years ago

I really gotta get some sleep. Til then.

Thanks for taking the time to investigate. When people ask me what they could contribute, this is pretty much it, investigating issues -- especially the ones I can't reproduce.

So at this point maybe a Pale Moon dev could provide some insights as to why getEnumerator('navigator:browser') returns only one window while no domwindowopened event is fired for the other ones.

DjogaRo commented 8 years ago

Not for that. That add-on itself is a huge chunk of work. What's investigating a problem, that bugs me, compared to building that thing in the first place. :) As soon as I knew, how I could investigate, there was no reason not to.

Yeah, I'm going to make a thread at PM's forums later. Maybe the API will get an update or there might be a well trodden way to handle the given behavior. We'll see.

DjogaRo commented 8 years ago

I was just about to report in the PM forum, when I recognized, that getting the initial windows utilizes another service than the registering an observer. Unification fixes the problem. Now my second window sometimes gets handled in the initial loop in start() and sometimes through an event by the observer. I'd yet have to see it handled in both codepathes during one startup.

Just replace

vAPI.tabs.getWindows = function() {
    var winumerator = Services.wm.getEnumerator(this.chromeWindowType);
    var windows = [];

with

vAPI.tabs.getWindows = function() {
    var winumerator = Services.ww.getWindowEnumerator(this.chromeWindowType);
    var windows = [];

In an example utilizing wm a listener for the window's load event was registered with the window whenever there's a new window opened. I have a feeling, that way all those delays may be unneeded. I'll investigate.

The example is the chosen answer in: http://stackoverflow.com/questions/29178158/in-a-firefox-restartless-add-on-how-do-i-run-code-when-a-new-window-opens-list

At PM forum I also asked, whether WindowWatcher or WindowMediator was to be preferred - respecting that the code is meant for all supported FF-lings. The thread is: http://forum.palemoon.org/viewtopic.php?f=16&t=9893 (I also asked to move the thread, and the way the url looks it may change by that).

gorhill commented 8 years ago

I recognized, that getting the initial windows utilizes another service than the registering an observer

Yes, I saw this yesterday, and wondered too if this could be the cause. Maybe there was a good reason to have been done this way, I will have to go through the commit history to see if this was like this since the beginning.

Once I release 1.3.1 I will revise the code and this will have to be well tested to find out if there are any side effect, including for Firefox for Android.

DjogaRo commented 8 years ago

Let me guess. Occasionally having two entries in the second window's context menu? When the second window gets handled by the initial loop in start and there's a delay because ( wintype !== 'navigator:browser' ) evaluates to true, it somehow happens, that it gets two CM entries.

I have yet to understand the magic behind those entries. With the same delays the first window still ends up with one entry. If I comment out the entry's registration in attachToTabBrowser, the first window still gets it - even the second window does sometimes.

Searching in files for "contextMenu.register" finds me no other call.

gorhill commented 8 years ago

Alright, I will push a new revision for uBO, now strictly using WindowMediator. I tested from Palemoon to Nightly, legacy toolbar button/Australis, and all seems to work just like before. Hopefully this will fix the issue on your side.

The doubling of the menu item is easily taken care of by first checking whether the item is already inserted.

gorhill commented 8 years ago

Can you confirm whether the fix work using latest uBO dev build?

DjogaRo commented 8 years ago

It's not here anymore. I've got to wait for the FF-release of 1.3.2b2, I guess.

gorhill commented 8 years ago

1.3.2b2 is available now in the Releases section.

DjogaRo commented 8 years ago

I can't see an xpi there. so I copied 1.3.0 to a new profile and replaced vapi-background.js with the on from 1.3.2b2. With that the issue still occurs from time to time. As soon as I get my hands on the new xpi or you tell me the mix I did suffices, I'll look into it.

DjogaRo commented 8 years ago

I'm on it

gorhill commented 8 years ago

I can't see an xpi there

My bad, I failed to notice the build failed for some reasons.

DjogaRo commented 8 years ago

Nevermind. The second window sometimes is neither handled in the initial loop nor does onOpenWindow get called. And still it gets a context menu entry. That's way above me.

Is WindowMediator to be prefered over WindowWatcher? Since using the other one, initial loop and event handling complemented each other quite good.

gorhill commented 8 years ago

I suppose both could be used, but I would have to review the code in there to be sure the it's proofed against multiple requests to attach to a window -- i.e. check if whatever needs to be done is already done.

gorhill commented 8 years ago

And still it gets a context menu entry

Yes, because uBO's core code calls for the installation of the context menu entry explicitly.

DjogaRo commented 8 years ago

Well, I had already tried (with WW) making the observer add an event listener to the new window - which is to wait for a "load" event. The listener function I had defined within observe(...), which I couldn't figure out to do in start(), too. That way onWindowLoad(...) when called by the observer never showed delays happening in the attach... functions.

So now I added

    var getOnLoadListener = function(win) {
        function onLoadListener() {
            console.log("__LL: Got called for a window done loading!");
            win.removeEventListener("load",onLoadListener);
            // Only add UI changes if this is a browser window
            if (win.document.documentElement.getAttribute("windowtype") === "navigator:browser") {
                console.log("__LL: Event domwindowopened");
                onWindowLoad(win);
            } else
                console.log("__LL: windowtype ain't navigator:browser!");
        }
        return onLoadListener;
    }

and replaced onWindowLoad(win); with win.addEventListener("load", getOnLoadListener(win)); so that I can use that in start, too. Having two context menu entries in window 2 only happened when handled in start and later experiencing delays. This should fix it plus make the delay code obsolete.

But now. I never see the initial loop iterated twice. Instead, I get

[20:31:40.211] this._cps2 is undefined @ chrome://browser/content/browser.js:1614
[20:31:40.405] this._lastStatus is null @ chrome://browser/content/browser.js:14018
[20:31:40.406] this._cps2 is undefined @ chrome://browser/content/browser.js:1614
[20:31:40.407] this._lastStatus is null @ chrome://browser/content/browser.js:14018

or one handled through the loop and one through the observer and then all goes fine. I don't understand how these errors in browser.js come about, but I've got a feeling, I have to look for them using the unchanged 1.3.2b2 again.

gorhill commented 8 years ago

This should fix it plus make the delay code obsolete.

The delay code has to stay. I just want to know if the latest b3 solves the issue.

DjogaRo commented 8 years ago

I'll have a look. By the way, I've reverted 1.3.2b2 back to before my additions. Those errors in browser.js always happen - when buttonwise all is fine and when not.

DjogaRo commented 8 years ago

Still missing the button in the 2nd window sometimes. Looking at the code changes I don't get which issue you were addressing.

gorhill commented 8 years ago

Looking at the code changes I don't get which issue you were addressing.

I think I am getting lost in the info here. Isn't true that not all windows are reported in the event handler? I thought you were saying here that both windowWatcher and windowMediator together would result in all windows being reported to onWindowLoad().

DjogaRo commented 8 years ago

I'm get lost, too. :)

No, mixing those two doesn't work here, cause using

vAPI.tabs.getWindows = function() {
    var winumerator = Services.wm.getEnumerator(this.chromeWindowType);

for startup never yields more than one window, for whatever reason. Using WindowWatcher for that, too, gives also those windows during startup, which won't cause a notification to a registered observer later. So only using WW, restored session windows are early enough generated to get them initially or certainly handled later.

In 1.3.2b2 you tried, if both places utilizing WM would also fit together, but it didn't. At least not on my machine.

And me getting lost shows in that I had forgotten to replace wm when getting the enumerator. (Update: Here I'm referring to my own attempt, I reported on, an hour ago or so) Fixed this. Now all is fine (using onLoadListener), except for that sometimes now the first window gets no button. So I managed to invert the problem. :)

And I noticed there are some more places utilizing either Services.wm or Services.ww. And I have no idea, where this mixture doesn't matter and where it might.

DjogaRo commented 8 years ago

Regarding my own attempt, I suspect, that the first window sometimes already fired the "load" event, when I'm registering for that. At least that would explain my outcome. So now I'm trying to find out how to test for the event having already been fired to call onWondowLoad directly. Or I could do the latter everytime in the loop's first iteration.

Eitherway, I'll report back. And btw, I didn't throw out the delay code, I keep that spiked with logging. In the end I want to know, if what I try is usefull.

DjogaRo commented 8 years ago

It seems to work fine. I haven't tested excessively, but so far I only saw exactly one context menu entry per window. Same goes for the toolbar button. And the log shows no going down the delay codepaths. If you want to take a look: https://gist.github.com/DjogaRo/15b6527ecdd6da278ec6

Testing on legacy FF I'll do at a later time.

I'm off to see Morpheus now.

gorhill commented 8 years ago

I already rewrote the window watching code. Will push a new build tomorrow.

gorhill commented 8 years ago

In latest build, I use both winddowMediator and windowWatcher's enumerators and event handlers, so there should be no cracks left for the events to fall through.

DjogaRo commented 8 years ago

I think I'll be home in one or two hours. I'll test it then.

DjogaRo commented 8 years ago

There again were instances of the missing button. So I wanted to come back already knowing, where it comes from. I again added logging and a minor change to test my suspect - here's the diff (basis was uBO 1.3.2b5): https://gist.github.com/DjogaRo/c5764a5469c8c4be54be and a would have been problematic PM startup's logging output (the wording is not to be taken too seriously): https://gist.github.com/DjogaRo/b99b2ee995727d8ee176 (the 3rd window is the browser console)

With the tests you apply to the results, the window watcher's enumerator gives you, you skip the window you are after. It's to early for the window to satisfy those tests - at least on my machine.

Using it exactly like the other one (enumerators) should do the trick. And why not Services.ww.getWindowEnumerator(chromeWindowType)?

I guess it doesn't matter that early, but with the 2nd enumerator you change the ids of the windows you've already got.