Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#2689 closed defect (incomplete)

Logic problem in BrowserAction._addChange

Reported by: tschuster Assignee:
Priority: Unknown Milestone:
Module: Platform Keywords:
Cc: sebastian, trev Blocked By:
Blocking: Platform: Chrome
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29317030/

Description

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

Change History (8)

comment:1 Changed 4 years ago by tschuster

  • Platform changed from Unknown to Chrome

comment:2 Changed 4 years ago by tschuster

  • Component changed from Unknown to Extensions-for-Adblock-Plus
  • Review URL(s) modified (diff)

comment:3 Changed 4 years ago by tschuster

  • Component changed from Extensions-for-Adblock-Plus to Core

comment:4 Changed 4 years ago by sebastian

  • Cc sebastian added
  • Component changed from Core to Platform

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.

comment:5 Changed 4 years ago by sebastian

  • Resolution set to incomplete
  • Status changed from new to closed
  • Tester set to Unknown

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.

comment:6 Changed 4 years ago by 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.

Last edited 4 years ago by trev (previous) (diff)

comment:7 Changed 4 years ago by trev

  • Cc Trev added

comment:8 Changed 4 years ago by trev

  • Cc trev added; Trev removed
Note: See TracTickets for help on using tickets.