Logic problem in BrowserAction._addChange

[tschuster]

Bill found this logic problem in ABP for Chrome:

​https://hg.adblockplus.org/adblockpluschrome/file/a491fec9ef12/chrome/ext/background.js#l225

I get an exception saying that this._changes is null. If you look at the code, _queueChanges calls _applyChanges (in the else branch), and the last thing _applyChanges does is set this._changes to null.

I am going to upload this patch for coderevie

[sebastian]

Replying to tschuster:

I get an exception saying that this._changes is null.

Steps to actually reproduce this would be helpful.

If you look at the code, _queueChanges calls _applyChanges (in the else branch), and the last thing _applyChanges does is set this._changes to null.

This doesn't explain anything. In addChange the _changes property is set to an object just before _queueChanges is called. And _queueChanges calls _applyChanges exactly once. So unless there is a bug in Chrome, I don't see any scenario where _changes is accessed while it is null.

[sebastian]

Can be reopened, when steps to actually reproduce this issue, or at least any attempt to proof that there is a logical issue has been provided.

[trev]

So the logic flaw is apparently: _addChange() assumes that chrome.tabs.get() will run asynchronously. If it happens to run synchronously then there will be a problem. The context of this is Mozilla's WebExtensions apparently, not Chrome.

Frankly, I think that this is an API issue. An API that was designed to execute asynchronously just shouldn't run synchronously, regardless of whether it could do so technically.