Simplified and removed memory leaks from code dealing with frames on Chrome

This fixes a few issues with the code dealing with frames on Chrome, while it also reduces that code by half:

1. We used an array instead of an object, accessed by frame ID, resulting in an array with a lot of empty elements.

2. We didn't flushed the previously seen frames, when the main frame was reloading or loading another page. That resulted in constantly increasing memory usage, while navigating within the same tab.

3. We are used to fallback to the top level frame when encountering an unknown frame. But this fall back failed sometimes after reloading the extension, because we didn't retrieved the URLs of tabs, already there on startup.

4. I was able to eliminate the Frame constructor and prototype (and a lot of unnecessary lookups) by just storing the frame data in the format expected by the high level code, in the first place.

[Wladimir Palant, 2014-03-06 15:32:20]

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

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:200: return framesOfTabs[tabId][frameId];
What about tabs we don't know?

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:210: // unfortunatelly we can't retrieve frames already
loaded.
We can: chrome.webNavigation.getAllFrames().

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:213: for (var i = 0; i < tabs.length; i++)
Nit: Please always put brackets around multiline blocks, for better readability.

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:254: frame = frames[frameId] || frames[0];
This will error out if framesOfTabs[details.tabId] doesn't exist. You seem to be
assuming that it will always exist because you query existing tabs on startup
and we record all main_frame calls. This won't be sufficient however.

1) chrome.tabs.query() takes time to execute. In the meantime we might get
requests for some tab.

2) We don't get all main_frame requests. For example, we don't receive anything
for https://chrome.google.com/webstore/, and neither for data: URLs. We might
still get requests for subframes however.

So the code initializing framesOfTabs[details.tabId] and
framesOfTabs[details.tabId][0] when missing is still required.

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:275: var frames = framesOfTabs[sender.tab.id];
Same here: please consider the scenario where we don't know that tab.

[Sebastian Noack, 2014-03-06 19:54:49]

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

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:200: return framesOfTabs[tabId][frameId];
On 2014/03/06 15:32:20, Wladimir Palant wrote:
> What about tabs we don't know?

Done.

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:210: // unfortunatelly we can't retrieve frames already
loaded.
On 2014/03/06 15:32:20, Wladimir Palant wrote:
> We can: chrome.webNavigation.getAllFrames().
Cool. Done.

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:213: for (var i = 0; i < tabs.length; i++)
On 2014/03/06 15:32:20, Wladimir Palant wrote:
> Nit: Please always put brackets around multiline blocks, for better
readability.

Done.

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:254: frame = frames[frameId] || frames[0];
On 2014/03/06 15:32:20, Wladimir Palant wrote:
> This will error out if framesOfTabs[details.tabId] doesn't exist. You seem to
be
> assuming that it will always exist because you query existing tabs on startup
> and we record all main_frame calls. This won't be sufficient however.
> 
> 1) chrome.tabs.query() takes time to execute. In the meantime we might get
> requests for some tab.

No, because I registered listeners for onBeforeRequest and onMessage after we
retrieved the existing tabs.

> 2) We don't get all main_frame requests. For example, we don't receive
anything
> for https://chrome.google.com/webstore/, and neither for data: URLs. We might
> still get requests for subframes however.

I also considered this case, but wasn't able to reproduce it. So I assumed that
if Chrome hides the top level frame from the extension, that it also hides all
subsequent requests for that frame.

> So the code initializing framesOfTabs[details.tabId] and
> framesOfTabs[details.tabId][0] when missing is still required.

Done.

http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chro...
chrome/ext/background.js:275: var frames = framesOfTabs[sender.tab.id];
On 2014/03/06 15:32:20, Wladimir Palant wrote:
> Same here: please consider the scenario where we don't know that tab.

Done.

[Wladimir Palant, 2014-03-06 20:58:18]

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

http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chro...
chrome/ext/background.js:224: parent: parentFrameId != -1 ?
frames[parentFrameId] : null
I don't think that parent frames are guaranteed to be listed earlier, nothing on
it in the docs. I'm afraid two passes are necessary here, with the parent
property being set in the second pass.

[Sebastian Noack, 2014-03-06 21:11:55]

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

http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chro...
chrome/ext/background.js:224: parent: parentFrameId != -1 ?
frames[parentFrameId] : null
On 2014/03/06 20:58:18, Wladimir Palant wrote:
> I don't think that parent frames are guaranteed to be listed earlier, nothing
on
> it in the docs. I'm afraid two passes are necessary here, with the parent
> property being set in the second pass.

I tested it and it lists the frames top-down. But yes, the documentation doesn't
say anything about that. So what do you suggest? I would also prefer to avoid
adding a second pass.

[Wladimir Palant, 2014-03-21 14:17:35]

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

http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chro...
chrome/ext/background.js:224: parent: parentFrameId != -1 ?
frames[parentFrameId] : null
I think a second pass is exactly what we should implement. You *could* do it
without by maintaining queues for each parent that hasn't been seen yet and then
process them when you actually get to that parent. However, this will get
significantly more complicated than a two-pass approach and not necessarily more
efficient (note that this isn't a hot spot so the minimal performance difference
doesn't matter). Add to this that this approach won't be properly testable
because as you already noted - the way this is currently implemented in Chrome
we should never hit an unknown parent.

[Sebastian Noack, 2014-03-31 15:43:44]

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

http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chro...
chrome/ext/background.js:224: parent: parentFrameId != -1 ?
frames[parentFrameId] : null
On 2014/03/21 14:17:35, Wladimir Palant wrote:
> I think a second pass is exactly what we should implement. You *could* do it
> without by maintaining queues for each parent that hasn't been seen yet and
then
> process them when you actually get to that parent. However, this will get
> significantly more complicated than a two-pass approach and not necessarily
more
> efficient (note that this isn't a hot spot so the minimal performance
difference
> doesn't matter). Add to this that this approach won't be properly testable
> because as you already noted - the way this is currently implemented in Chrome
> we should never hit an unknown parent.

Done.

[Wladimir Palant, 2014-04-01 11:16:33]

LGTM