Closed Bug 1448674 Opened 4 years ago Closed 4 years ago

Port.onDisconnect fired with tabs.onAttached

Categories

(WebExtensions :: General, defect, P1)

61 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- verified

People

(Reporter: baptiste.themine, Assigned: robwu)

References

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171024165158

Steps to reproduce:

A WebExtension contains an option page which communicates with the background script as below :
- option page script call browser.runtime.connect
- background script listen to browser.runtime.onConnect

1. Open the option page of extension.
2. Detach the tab containing option page from main window.
3. Attach the tab to the main window.


Actual results:

When the option page is attached to the window i.e. when the event tabs.onAttached is fired, the event Port.onDisconnect is fired too and it is not possible to communicate with the background script anymore.

Until Firefox 59 : Port.onDisconnect was not fired with tabs.onAttached.
Firefox Beta 60 : UNCONFIRMED.
Firefox Nightly 61 : Port.onDisconnect is fired with tabs.onAttached.


Expected results:

Port.onDisconnect should not fire with tabs.onAttached such as in previous version of Firefox.
Caused by https://hg.mozilla.org/mozilla-central/rev/546f73d3fc88

This is similar to bug 1445537, which was fixed by https://hg.mozilla.org/mozilla-central/rev/6ed4300a066b
That commit does not fix this bug, because the SwapDocShells listener is only attached to the initial browser, and not re-attached after swapping the docshell.

I'll work on a fix.
Assignee: nobody → rob
Blocks: 1444758
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
Moving this issue into the "WebExtensions: General" bugzilla component, I can't see anything in this issues that seems related to the "WebExtensions: Developer Tools" component (as it is not related to the neither the devtools API or the addon debugging tools).
Component: WebExtensions: Developer Tools → WebExtensions: General
Priority: -- → P1
I confirm that Firefox 60 is not affected by this issue.
Comment on attachment 8965658 [details]
Bug 1448674 - avoid closing extension ports while detaching tabs

https://reviewboard.mozilla.org/r/234510/#review240702

Why did you add the extension page test to an existing test but write a brand new test for content scripts?  I think a new test for extension pages would make more sense...

::: browser/components/extensions/test/browser/browser_ext_contentscript_connect_and_move_tabs.js:32
(Diff revision 2)
> +          } catch (e) {
> +            browser.test.fail(`Error: ${e} :: ${e.stack}`);
> +            browser.test.notifyFail("contentscript_port.postMessage");
> +            return;
> +          }
> +          port.disconnect();

Why is this here?  I'm a little confused by the flow but it looks like the disconnect is meant to happen in response to the disconnect-me message a few lines below

::: browser/components/extensions/test/browser/browser_ext_contentscript_connect_and_move_tabs.js:42
(Diff revision 2)
> +          port.disconnect();
> +          // Now port.onDisconnect should fire in the content script.
> +        });
> +      });
> +
> +      browser.tabs.create({url: "http://mochi.test:8888/"}, tab => {

please make background async and use await here

::: browser/components/extensions/test/browser/browser_ext_tab_runtimeConnect.js:31
(Diff revision 2)
> +            await browser.windows.create({tabId});
> +            await browser.tabs.move(tabId, {index: 0, windowId});
> +            await browser.windows.create({tabId});
> +            await browser.tabs.move(tabId, {index: 0, windowId});

why do this twice?
Comment on attachment 8965658 [details]
Bug 1448674 - avoid closing extension ports while detaching tabs

https://reviewboard.mozilla.org/r/234510/#review240702

I piggy-backed on an existing test because a new test would also have followed the same test structure (if I create a separate test file with the minimal reproduction case, then the file would only be a tiny bit smaller, namely one ping-pong less).

I tried to add the test to browser_ext_contentscript_connect.js too, but created a new one instead because somehow the bug does not reproduce with that file. If I recall correctly, that was because of the two runtime.onConnect listeners.

> Why is this here?  I'm a little confused by the flow but it looks like the disconnect is meant to happen in response to the disconnect-me message a few lines below

Thanks, this line should be removed.

> please make background async and use await here

Ok.

> why do this twice?

For this specific bug, only once is sufficient, because with the current code, the following states exist:
1. Initial state: tab with initial message manager
2. receiveMessage state: message manager is swapped via the listener that was added via "Extension:Connect" in receiveMessage.
3. handleEvent: message manager is swapped via handler of "SwapDocShells" in handleEvent.

windows.create tests transiton 1 -> 2 (which was fixed by Kris's patch on the other bug).
tabs.move tests transition 2 -> 3 (which is fixed by this bug).

Repeating windows.create tests transition 3 -> 3.
Then I repeat tabs.move again to restore the UI to the state at the start of the test.
Comment on attachment 8965658 [details]
Bug 1448674 - avoid closing extension ports while detaching tabs

https://reviewboard.mozilla.org/r/234510/#review241062

Looks good.  Please add a brief comment at the top of the test explaining what it being tested (i.e., "Tests that a Port works properly and is not prematurely disconnected as the underlying message managers change when a tab is moved between windows")

::: browser/components/extensions/test/browser/browser_ext_connect_and_move_tabs.js:58
(Diff revision 3)
> +      `,
> +      "script.js": function() {
> +        let port = browser.runtime.connect();
> +        port.onMessage.addListener(msg => {
> +          browser.test.assertEq("ping", msg, "expected message");
> +          browser.test.notifyPass("port_ping_ponged_before_disconnect");

nit: just use sendMessage() here, there's not much difference in practice -- this conventionally used as the last thing in the test, but this test isn't quite finished at this point.
Attachment #8965658 - Flags: review?(aswan) → review+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/b35a42daaf1e
avoid closing extension ports while detaching tabs r=aswan
https://hg.mozilla.org/mozilla-central/rev/b35a42daaf1e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Attached file Bug1448674.zip
I was able to reproduce this issue on Firefox 61.0a1 (20180325100345) under Win 7 64-bit and Mac OS X 10.13.2.

This issue is verified as fixed Firefox 61.0a1 (20180425100122) under Win 7 64-bit and Mac OS X 10.13.3.

I was not able to reproduce this issue on Firefox 59.0.2 (20180323154952) and Firefox 60.0b15 (20180423171039) under Win 7 64-bit and Mac OS X 10.13.2.

Please have a look at the attached video.

Thanks Rob!
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.