1642967 - Marking feeds as read in Feedbro RSS reader extension does not decrease unread number in sidebar

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[gwarser]

• install https://addons.mozilla.org/en-US/firefox/addon/feedbroreader/
• add feed https://www.reddit.com/r/all/.rss
• click on feed in sidebar
• click on "trash" or "O" icon to remove or make as read
• number in sidebar does not decrease

26:50.52 INFO: Downloading build from: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Sd1X60QqQUKrsf7ZDi-6tA/runs/0/artifacts/public%2Fbuild%2Ftarget.tar.bz2
===== Downloaded 100% =====
27:47.35 INFO: Running autoland build built on 2020-06-01 16:46:03.924000, revision e8d0f18c
27:58.83 INFO: Launching /var/tmp/tmppvwkrdnb/firefox/firefox
27:58.83 INFO: Application command: /var/tmp/tmppvwkrdnb/firefox/firefox --allow-downgrade -profile /var/tmp/tmp9r9mnqgi.mozrunner
27:58.84 INFO: application_buildid: 20200601150729
27:58.84 INFO: application_changeset: e8d0f18c2300a223c31847d27196d1a8ad673515
27:58.84 INFO: application_name: Firefox
27:58.84 INFO: application_repository: https://hg.mozilla.org/integration/autoland
27:58.84 INFO: application_version: 78.0a1
Was this integration build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): good
28:14.28 INFO: Narrowed integration regression window from [a540f1bf, ca3057f6] (4 builds) to [e8d0f18c, ca3057f6] (2 builds) (~1 steps left)
28:14.28 INFO: No more integration revisions, bisection finished.
28:14.28 INFO: Last good revision: e8d0f18c2300a223c31847d27196d1a8ad673515
28:14.28 INFO: First bad revision: ca3057f6cdc555f3ddfb1f10afa6805a43253a85
28:14.28 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e8d0f18c2300a223c31847d27196d1a8ad673515&tochange=ca3057f6cdc555f3ddfb1f10afa6805a43253a85

Comment 1

[Rob Wu [:robwu]]

(I didn't confirm, but given the mozregression output, it makes sense to link the two bugs)

Comment 2

[Rob Wu [:robwu]]

(still not verified, but I did some code expection).

The extension adds multiple runtime.onMessage listeners and uses the sendResponse callback. That made me wonder whether our implementation ignores messages without response.

The updated finalizer rejects the promise: https://searchfox.org/mozilla-central/rev/559b25eb41c1cbffcb90a34e008b8288312fcd25/toolkit/components/extensions/ExtensionChild.jsm#129-150

... but it doesn't seem that we ignore it here: https://searchfox.org/mozilla-central/rev/559b25eb41c1cbffcb90a34e008b8288312fcd25/toolkit/components/extensions/ConduitsParent.jsm#285

I created a STR and will attach it shortly. The STR consists of two runtime.onMessage listeners. One should return a promise that never resolves (or equivalently: return true; without calling sendResponse) and the other should delay the response until the first one has been GCd.

Comment 3

[Rob Wu [:robwu]]

Attachment: sendResponse-gc.zip

STR:

1. Load attached extension at about:debugging. It will open a new extension tab.
2. Open the global JS console (ensure that it logs messages from content).
3. Click on the button inside the extension page.
4. Wait a bit (or go to about:memory and "Minimize memory usage" to force GC)
5. Click on the extension button to manually send the response.
6. Look at the global JS console, after "Listener 2 - resolve when browserAction is clicked"

Expected:

• "Listener 2 will be handled"
• "MSG: response is want reply"

Actual:

• "MSG: sendMessage rejected Error: Promised response from onMessage listener went out of scope" (after step 4)
• "Listener 2 will be handled"

Comment 4

[Tomislav Jovanovic :zombie]

(In reply to Rob Wu [:robwu] from comment #3)

STR:

Unless I'm misunderstanding something, I get the same result on current Beta 78 as described above, so not sure that's the difference.

Comment 5

[Rob Wu [:robwu]]

Comment on attachment 9153862 [details]
sendResponse-gc.zip

Apparently it's a pre-existing bug, which I discovered by looking at code. I've moved it to bug 1643186.

I guess that we actually need to run the extension from the report and debug it.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1583484

Comment 7

[voltron]

Here's a minimal test case:

1. Install Feedbro
2. Open Feedbro Reader view
3. Open the background console Inspect view
4. Give this command in the console:
   chrome.tabs.query({title: "Feedbro"}, function(tabs) { chrome.tabs.sendMessage(tabs[0].id, {}); });

-> Error:
Unchecked lastError value: Error: Could not establish connection. Receiving end does not exist.

Note that this is obviously not Feedbro related. The bug can be repeated with any webextension.

Looks like chrome.tabs.sendMessage is broken.

Comment 8

[Tomislav Jovanovic :zombie]

Thanks for the minimal STR, very helpful!

Comment 9

[voltron]

Attachment: firefox-nightlybug-1642967.zip

Here's a minimal test case as a standalone minimal webextension.

1. Install the webextension
2. Open the background console (Inspect)
3. Run command: test();
4. Go back to the background console and see the error

Comment 10

[Tomislav Jovanovic :zombie]

Attachment: Bug 1642967 - tabs.sendMessage can also target extension pages

Comment 11

[Tomislav Jovanovic :zombie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d9f04aa6632028a10f4401a95c3270b04864a8e

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fb8cf0f6d60
tabs.sendMessage can also target extension pages r=robwu

Comment 14

[Razvan Maries]

Backed out for perma failures on test_ext_test.html.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=IBtf8CqtRDi7qAwpC_4sDg-0&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=android%2C7.0%2Cx86-64%2Copt%2Cmochitests%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-mochitest-plain-e10s-1%2Cm%281%29&revision=9fb8cf0f6d60ff809bcaa3ebc1ed29ef41c62c9e

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305345495&repo=autoland&lineNumber=6009

Backout: https://hg.mozilla.org/integration/autoland/rev/7edbb956e164818ee37f09412029db4f6b0d90ab

Comment 15

[Rob Wu [:robwu]]

text_ext_text.html is failing because it is running the content script logic twice, due to a stray test_sample.html from a previous test.

In logcat (logcat-emulator-5554.log, line 132346) I can see that test_ext_tabs_sendMessage.html ends prematurely after window.open("file_sample.html"). This is because there is another file_sample.html that already triggered the content script of this test: the "Response from iframe" messaging came before "Response from top level", and the only way to observe that in the test is if there was a pre-existing file_sample.html window.

Because the "extension-frame-responded" had been sent before in the test, the end of the test is comparable to window.open("file_sample.html").close() (to be precise: with just one microtask between open and close).
I'm surprised that this somehow extended the lifetime of the window (or was it a previous file_sample.html window?) to the test_test_in_content_script part of the next test. But in any case, this is not an issue with the patch from this bug itself.

I suggest to reland without changes if the test succeeds on try, and otherwise skip it on Android for now.

The failure message is very misleading, and it would have been easier to spot if the test runner on GeckoView actually checks for stray windows after each test (i.e. the "Test left extra windows or tabs:" message). That is part of bug 1565700.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c64311d6fbb9
tabs.sendMessage can also target extension pages r=robwu

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c64311d6fbb9

Comment 18

[Alex Cornestean]

Hello,

Verified the fix using the latest Nightly (79.0a1/20200617215206) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS. Also reproduced the issue on an older Nightly (79.0a1/20200601214228) for comparison purposes.

Tested with the original STR and also the ones from Comment 7 and on the latest Nightly scrolling through the feed content/marking articles as read/removing articles will properly decrease the number from the add-on icon in the toolbar and in the sidebar. Furthermore, the Error: Unchecked lastError value: Error: Could not establish connection. Receiving end does not exist. will not be displayed in the console.