[firefox] open a new website in an existing tab causes website to break

[harshanvn]

Steps To Reproduce

1. Configure uBlock in Default Deny and block 3P script & IFrame
2. Open any website in a new tab (say github.com)
3. In the same tab, now goto wilderssecurity.com

You will see wilderssecurity.com completely broken..

Here is the uBlock popup snapshot -

uBlock v0.9.2.4 - dev4
W8.1 64 bit - FF v37 32 bit

[gorhill]

Could not reproduce:

FF36 though, FF37 is not out for my OS.

[gorhill]

When you say

now goto wilderssecurity.com

How? Entering the address manually, link, bookmark...?

[Betsy25]

I had the same problem as @harshanvn when I drag&drop installed the latest .xpi (self compiled), it looks like a browser restart got rid of the problem & never experienced it again. (The count badge also showed huge blocking numbers 500+ and more.)
When I had, say, majorgeeks.com open, the popup showed the majorgeeks related items, when i then clicked another bookmark to open in that same tab, the content of the tab got broken, and the popup showed items of both majorgeeks and the newly opened site.
Looks like some code must have mangled using the restartless update method which required a browser restart to fix it permanently.

FF 40(nightly), Win7, one of the latest commits.

[gorhill]

@Betsy25 Maybe related to the code changes in b053f1e related to the listening of tab content change events. I do sometimes experience quirks after I try to update the package, so I've taken the habit now to remove uBlock, quit/restart Firefox, and drag-n-drop, it's more reliable I found.

[Betsy25]

@gorhill I honestly don't know anymore what the latest commit was, I'll try keeping the same update procedure and see if the misbehavior occurs again. Wouldn't mind myself if updating requires a browser restart, but i'll just try to see if it continues with newer commits.

[harshanvn]

@gorhill I am no longer able to reproduce the issue, after i restart by browser.

However, i did screen record the issue, before i restarted it. So, you can see the problem :)

[Betsy25]

@harshanvn Yep, that's how sites looked here too. I just drag&drop installed the latest commit and couldn't reproduce anymore. That's good.

[gorhill]

@harshanvn Ok, that's how I tried to reproduce, by typing the address in the address bar.

I suspect some changes sometimes require a browser restart, and for the commit mentioned above there were changes about using a new API for tab change events.

[harshanvn]

Ok Thanks. That sounds reasonable.

Strange thing (unrelated to this bug i think..), when you try open above give gif link directly...it open fine. However if i try to do ctrl-f5 to reload the page, it will not reload...

https://cloud.githubusercontent.com/assets/779630/6927159/14c1b692-d809-11e4-900e-2893cb1c10f5.gif

Does it work fine for you in firefox..

[harshanvn]

Strange thing (unrelated to this bug i think..), when you try open above give gif link directly...it open fine. However if i try to do ctrl-f5 to reload the page, it will not reload...

https://cloud.githubusercontent.com/assets/779630/6927159/14c1b692-d809-11e4-900e-2893cb1c10f5.gif

Does it work fine for you in firefox..

Please ignore this. Looks like a firefox thing. Able to reproduce the issue even if ublock has been disabled. Maybe sth to do with v37 release...

[Betsy25]

https://cloud.githubusercontent.com/assets/779630/6927159/14c1b692-d809-11e4-900e-2893cb1c10f5.gif is unreachable at the moment for me.

[harshanvn]

you restart your browser and open the link. it will work fine for the first time and then the issue repeats from second reload onwards...

[gorhill]

Happens to me too. I see the image is hidden because it is detected as being blocked by the content script, though it is not blocked by the network requests handler. Investigating.

[gorhill]

If whitelisting about-scheme, the issue does not arise. So it's yet another Firefox navigation problem. Will see if it existed without the latest changes.

[gorhill]

I am unable to reproduce with 0.9.2.3. @harshanvn can you open a new issue for this? Looks like the issue arise because of #1135.

Actually, I believe the issue here could very well be explained by the image problem as well.

[gorhill]

@AlexVallat , @Deathamns

Repro steps:

• Advanced user mode
• Globally block 3rd parties
• Un-whitelist about-scheme if not done yet.
• Click the GIF in this comment: gorhill/uBlock#1140 (comment).
• Sometimes, the image won't display because it will be deemed 3rd-party by uBlock, because the tab is still bound to about:blank by the time the content scripts execute.

The fact that the image is not blocked as a network request but is blocked when queried by the content script is because the image is requested as a root document, which obviously can't ever be filtered as a 3rd-party resource, but the content script query result of filtering as an image.

I could not reproduce the issue in 0.9.2.3, so likely related to #1135.

[AlexVallat]

Argh, some nasty timings going on here. OK, so it looks like if nsIObserver.observe for document-element-inserted is called, it's called before nsIWebProgressListener.onLocationChange is called, which is a little irritating.

What we could do is insert a sync message to ensure that a location changed is registered before running the content scripts, so in frameModule.js to the docReady function add:

            let messageManager = doc.docShell.getInterface(Ci.nsIContentFrameMessageManager);

            messageManager.sendSyncMessage(locationChangedMessageName, {
                url: loc.href,
            });

just before loading the contentscript-end.js.

We still need the location change listener too, though, because document-element-inserted is not called at all if the page is loaded from session cache (navigating backwards and forwards, for example). This means we will usually get two location changes for the same page, which isn't ideal either. I'm not sure how much of a problem this would be, if it is worth trying to set some sort of flag that document-element-inserted has already raised locationChanged so onLocationChange should skip it? That's the sort of fragile hack that usually ends up causing more problems, though.

Edit: Packaged the code fragment I described into a pull request for convenience: #1150

[gorhill]

Just some thoughts about the latest regressions.

I have always seen stability as more important than "minimize code duplication", and it's why I was more of the opinion to have a dedicated Fennec branch, so that any code change to it would have zero effect on the Firefox branch. I was planning a release next weekend, but it doesn't look like this will happen. Adding more hacks to patch unexpected side effects is likely not a stabilizing move.

So.. I have to say now I wonder if it is time to split Firefox/Fennec in two, put back the Firefox branch into the state where all was working as expected, and from now on work on Fennec with no longer the fear there will be any side effect to the Firefox version -- and vice versa.

[AlexVallat]

I see where you're coming from. There are disadvantages to having separate Fennec and Desktop builds too, of course, and I'm not sure what AMO can offer by way of linking listings, but in the end you've got to weigh it all up in consideration.

I will say that these latest changes, to move to a nsIWebProgressListener, were not simply for Fennec but were an attempt at undoing a previous hack (triggering an onNavigation as a result of TabSelect) for Desktop. The alternative would have been more layers of hacks on top of it, even if separately for Fennec and Desktop. It would just mean different hacks for the two of them, which isn't necessarily an improvement!

[chrisaljoudi]

@AlexVallat can't reproduce anymore, so that's a good sign.

[AlexVallat]

It should, at a minimum, fix the issue. My only concern is whether it causes other unexpected problems. Together with caea8c5, I'm pretty sure it won't have any ill effect when onLocationChange occurs before observe (as is good and natural), but in rare cases such as the gitHub gif link here, when they arrive reversed, it will have the side effect that two location changes will be raised for the same page. I think the worst that could happen would be a slightly inaccurate badge count, as it would reset the counter with the second call. Unless there's something else horrible I've missed, I'd be happy to call this one closed.