Baptiste Thémine Reporter
	Description • 6 years ago

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.

	Rob Wu [:robwu] Assignee
	Comment 1 • 6 years ago

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.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 2 • 6 years ago

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).

	Baptiste Thémine Reporter
	Comment 5 • 6 years ago

I confirm that Firefox 60 is not affected by this issue.

	Andrew Swan [:aswan]
	Comment 6 • 6 years ago
mozreview-review

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?

	Rob Wu [:robwu] Assignee
	Comment 7 • 6 years ago
mozreview-review-reply

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.

	Andrew Swan [:aswan]
	Comment 9 • 6 years ago
mozreview-review

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.

	Pulsebot
	Comment 11 • 6 years ago

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/b35a42daaf1e
avoid closing extension ports while detaching tabs r=aswan

	Andrei Ciure[:aciure]
	Comment 12 • 6 years ago
bugherder

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

	CosminB
	Comment 13 • 6 years ago

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!