Refactored the abstraction layer to address prerendered pages on Safari caused by leaky abstraction

Prerendered pages on Safari 7 are exposed with the same SafariBrowserTab object as the page currently shown in the tab. This leads to a bunch of issues on Safari 7 at the moment, most notable messing up the ad counter.

This is caused by a leaky abstraction. We just map SafariBrowserTab objects to tabs in the abstraction layer. While we do something similar on Chrome, prerendered pages are already exposed as separate tabs by Chrome's API. So in order to fix this, we also must expose prerendered pages on Safari separately.

Since our top-level code isn't actually interested in tabs and windows, but in the pages running inside them, I refactored the abstraction layer and moved from a mindset based on windows and tabs to a mindset based on pages.

[Wladimir Palant, 2014-04-04 14:00:34]

Only got to safari/ext/background.js today, posting the comments so far.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/back...
File background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/back...
background.js:256: ext.pages.query({lastFocusedWindow: true}, function(pages)
The compat info for Chrome needs to be changed - lastFocusedWindow parameter is
only supported since Chrome 19, currently we still allow Chrome 18.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/back...
background.js:272: ext.pages.open(optionsUrl, callback);
Won't this break abp: links? The add-subscription message handler will now open
the tab and forward the message to it immediately - the tab might not be loaded
at that point and won't be able to handle the message.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
chrome/ext/background.js:104: if (visibleWindow != (win.state != "minimized"))
That's quite a bit of code just to exclude minimized windows. Do we really need
it?

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
chrome/ext/background.js:144: });
Just wondering: will this also be triggered for navigation to a javascript: URL?

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
File ext/background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
ext/background.js:3: * Copyright (C) 2006-2013 Eyeo GmbH
Note: Please update this when rebasing.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
ext/background.js:20: var nonEmptyPageMaps = {__proto__: null};
Note: we will leak every PageMap object ever created so the assumption here is
clearly that PageMap objects are always persistent. That issue was already
present before that change however, and the assumption currently seems to be
true - still non-obvious however and worth a comment.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
ext/background.js:34: delete nonEmptyPageMaps[this._id];
This isn't really great from the performance point of view, an empty PageMap
also isn't exactly a common scenario. How about dropping the "non empty"
constraint and simply tracking all maps (in a regular array then)?

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
File ext/common.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
ext/common.js:31: this._listeners.push(listener);
Please check first whether this listener is already in the list, just in case.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/noti...
File notification.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/noti...
notification.js:88: ext.pages.open(link.href);
Please use spaces for indentation.

[Sebastian Noack, 2014-04-07 13:15:25]

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/back...
File background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/back...
background.js:256: ext.pages.query({lastFocusedWindow: true}, function(pages)
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> The compat info for Chrome needs to be changed - lastFocusedWindow parameter
is
> only supported since Chrome 19, currently we still allow Chrome 18.

Done.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/back...
background.js:272: ext.pages.open(optionsUrl, callback);
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> Won't this break abp: links? The add-subscription message handler will now
open
> the tab and forward the message to it immediately - the tab might not be
loaded
> at that point and won't be able to handle the message.

It was wishful thinking, that the callback passed to chrome.tabs.create(), isn't
called before the tab is loaded and ready to receive messages. But you are
right, this isn't the case. I have addressed that inside ext.pages.open().

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
chrome/ext/background.js:104: if (visibleWindow != (win.state != "minimized"))
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> That's quite a bit of code just to exclude minimized windows. Do we really
need
> it?

We need that for the icon animation, in order to animate the icon only for
visible tabs if any. That code isn't actually new, it was just moved from
iconAnimation.js to chrome/ext/background.js.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
chrome/ext/background.js:144: });
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> Just wondering: will this also be triggered for navigation to a javascript:
URL?

It isn't triggered for javascript: URLs entered into the address bar.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
File ext/background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
ext/background.js:3: * Copyright (C) 2006-2013 Eyeo GmbH
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> Note: Please update this when rebasing.

Done.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
ext/background.js:20: var nonEmptyPageMaps = {__proto__: null};
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> Note: we will leak every PageMap object ever created so the assumption here is
> clearly that PageMap objects are always persistent. That issue was already
> present before that change however, and the assumption currently seems to be
> true - still non-obvious however and worth a comment.

We only keep track of non-empty PageMaps, and pages are automatically removed
from the PageMap, when tabs are closed, reloading, or navigating to a different
URL.

So it is safe to discard PageMap objects without leaking any memory, if you
clear them before. And even if you don't, the leak will be just temporary, for
as long as the contained pages are open.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
ext/background.js:34: delete nonEmptyPageMaps[this._id];
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> This isn't really great from the performance point of view, an empty PageMap
> also isn't exactly a common scenario. How about dropping the "non empty"
> constraint and simply tracking all maps (in a regular array then)?

The purpose of this approach is that we don't have to leak every PageMap ever
created, see above.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
File ext/common.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/ext/...
ext/common.js:31: this._listeners.push(listener);
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> Please check first whether this listener is already in the list, just in case.

Done.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/noti...
File notification.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/noti...
notification.js:88: ext.pages.open(link.href);
On 2014/04/04 14:00:35, Wladimir Palant wrote:
> Please use spaces for indentation.

Done.

[Wladimir Palant, 2014-04-11 13:02:35]

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
chrome/ext/background.js:104: if (visibleWindow != (win.state != "minimized"))
On 2014/04/07 13:15:25, Sebastian Noack wrote:
> We need that for the icon animation, in order to animate the icon only for
> visible tabs if any. That code isn't actually new, it was just moved from
> iconAnimation.js to chrome/ext/background.js.

It seems that the original code was only animating the current tab so it didn't
care whether the window is minimized. As discussed on IRC, excluding minimized
windows doesn't seem to be useful - even it helps lower CPU consumptions in some
scenarios (users with lots of minimized windows?), that isn't really worth the
complexity here.

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
chrome/ext/background.js:144: });
I did some testing and it seems that using onBeforeNavigate as opposed to
onCommitted is sane.

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/background.js:52: pages[this._id]._tab.activate();
Why not this._tab.activate()?

If it is because multiple Page instances can exist and only the one in pages has
all the info - you can make the Page constructor return existing object from
pages if one exists. This will make sure that only one object exists for any
page.

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/background.js:200: if (this._page._tab == currentWindow.activeTab &&
!this._page._prerendered)
This should use isPageActive instead of duplicating the logic.

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/background.js:238: updateToolbarItemForPage(activePage,
event.target.browserWindow);
Please add a check for activePage - it might potentially be null.

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/background.js:401: };
Use .bind(this) instead of the proxy variable?

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
File safari/ext/common.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/common.js:166: if (substitutions && substitutions.constructor ==
Array)
Please remind me, why can't this be simply |substitutions instanceof Array|?

Note that this implementation is different from both Chrome and Firefox. Our
Firefox implementation always expects an array for substitutions, this is what
it should be per chrome.i18n.getMessage() documentation. The Chrome
implementation on the other hand seems to do something like this:

if (typeof substitutions == "string")
  substitutions = [substitutions];

Only one replacement is being done here and other types (e.g. numbers or even
String objects) result in an empty string.

[Sebastian Noack, 2014-04-11 14:47:44]

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5629499534213120/chro...
chrome/ext/background.js:104: if (visibleWindow != (win.state != "minimized"))
On 2014/04/11 13:02:35, Wladimir Palant wrote:
> On 2014/04/07 13:15:25, Sebastian Noack wrote:
> > We need that for the icon animation, in order to animate the icon only for
> > visible tabs if any. That code isn't actually new, it was just moved from
> > iconAnimation.js to chrome/ext/background.js.
> 
> It seems that the original code was only animating the current tab so it
didn't
> care whether the window is minimized. As discussed on IRC, excluding minimized
> windows doesn't seem to be useful - even it helps lower CPU consumptions in
some
> scenarios (users with lots of minimized windows?), that isn't really worth the
> complexity here.

Done.

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/background.js:52: pages[this._id]._tab.activate();
On 2014/04/11 13:02:35, Wladimir Palant wrote:
> Why not this._tab.activate()?
> 
> If it is because multiple Page instances can exist and only the one in pages
has
> all the info - you can make the Page constructor return existing object from
> pages if one exists. This will make sure that only one object exists for any
> page.

Done. When I began to write this code there was no Page._tab, and I just forgot
to change this line, when I added Page._tab.

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/background.js:200: if (this._page._tab == currentWindow.activeTab &&
!this._page._prerendered)
On 2014/04/11 13:02:35, Wladimir Palant wrote:
> This should use isPageActive instead of duplicating the logic.

Done.

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/background.js:238: updateToolbarItemForPage(activePage,
event.target.browserWindow);
On 2014/04/11 13:02:35, Wladimir Palant wrote:
> Please add a check for activePage - it might potentially be null.

In that case we have to reset the toolbar item properties to their default, as
done by updateToolbarItemForPage() if the given page is null.

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/background.js:401: };
On 2014/04/11 13:02:35, Wladimir Palant wrote:
> Use .bind(this) instead of the proxy variable?

I can't, since I also need the actual context the function is called in:

  contextId: proxy.registerObject(this, objects)

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
File safari/ext/common.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/common.js:166: if (substitutions && substitutions.constructor ==
Array)
On 2014/04/11 13:02:35, Wladimir Palant wrote:
> Please remind me, why can't this be simply |substitutions instanceof Array|?

When I wrote the original code I followed the top answer of a popular
StackOverflow question
(https://stackoverflow.com/questions/4775722/check-if-object-is-array).

However now where I know a lot more about JavaScript that approach seems weird
to me and I don't understand how this answer got over 600 points. So I skipped
the string conversion, and directly checked for the constructor.

The only difference between checking the constructor and using the isinstance
operator, is that the isinstance operator will also return true, if the object
is an instance of a subclass of Array. But that shouldn't be an issue here.

> Note that this implementation is different from both Chrome and Firefox. Our
> Firefox implementation always expects an array for substitutions, this is what
> it should be per chrome.i18n.getMessage() documentation. The Chrome
> implementation on the other hand seems to do something like this:
> 
> if (typeof substitutions == "string")
>   substitutions = [substitutions];
> 
> Only one replacement is being done here and other types (e.g. numbers or even
> String objects) result in an empty string.

The purpose of this code is to emulate Chrome's behavior, so this should be the
way to go. It might be that, I didn't tested chrome.i18n.getMessage() with
numbers, but were just expecting that they behave like strings, back when I
wrote the original code.

[Wladimir Palant, 2014-04-11 18:31:19]

LGTM

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
File safari/ext/common.js (right):

http://codereview.adblockplus.org/5464830253203456/diff/5634387206995968/safa...
safari/ext/common.js:166: if (substitutions && substitutions.constructor ==
Array)
On 2014/04/11 14:47:45, Sebastian Noack wrote:
> However now where I know a lot more about JavaScript that approach seems weird
> to me and I don't understand how this answer got over 600 points. So I skipped
> the string conversion, and directly checked for the constructor.

The only case where instanceof Array is problematic is when dealing with
different JavaScript contexts - each window and frame has its own Array
constructor and an array from another window cannot be checked against your
constructor. Not really something that is a concern here however.