New issue
Advanced search Search tips

Issue 660706 link

Starred by 1 user
Status: Fixed
Owner:
rdevlin....@chromium.org
Closed: Nov 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug-Regression



Sign in to add a comment

chrome.runtime.sendMessage() no longer works in 'unload' handler

Reported by thd...@gmail.com, Oct 30 2016 
UserAgent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.28 Safari/537.36

Steps to reproduce the problem:
1. Unzip test.zip
2. Go to chrome://extensions/ and load it as an unpacked extension
3. Click on "background page" link for this extension and monitor the Console tab
4. Go to any web page and reload it

What is the expected behavior?
The extension's content script should be able to send a message to the event page (background) for both 'load' and 'unload' events.

What went wrong?
The extension's content script was only able to send a message to the event page for the 'load' event. I can confirm that chrome.runtime.sendMessage() used to work from the 'unload' event handler as recent as Chrome v53, and possibly also v54, because my extension was working just fine then. The component that relied on chrome.runtime.sendMessage() from inside the 'unload' event handler only started breaking since v55.

Did this work before? Yes v53 and possibly v54

Does this work in other browsers? N/A

Chrome version: 55.0.2883.28  Channel: beta
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: Shockwave Flash 23.0 r0
test.zip
1.7 KB Download

Comment 1 by woxxom@gmail.com, Oct 30 2016 

Bisect: 417775 (good) - 417785 (bad)
https://chromium.googlesource.com/chromium/src/+log/64a06144..bf856ef5?pretty=fuller
Suspecting https://crrev.com/2300453002 as the only extension-related change.

I'm also attaching the above extension modified with "alert" on unload event instead of "console.log" for easier assessment of the result.

It should be noted that "beforeunload" event works correctly.
test.7z
816 bytes Download

Comment 2 by brajkumar@chromium.org, Oct 31 2016

Components: Platform>Extensions
Labels: hasbisect
Owner: rdevlin....@chromium.org
Status: Assigned (was: Unconfirmed)
As per comment #1 bisect info assigning to the concerned dev for more updates.

Comment 3 by rdevlin....@chromium.org, Nov 11 2016

Labels: ReleaseBlock-Stable
Migrating to async port creation wasn't supposed to break anything.  I'm going to see about fixing this and merging back before M55 hits stable.

Comment 4 Deleted

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 14 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69424807aa1ee34516783d9dd206a95e9f034e33

commit 69424807aa1ee34516783d9dd206a95e9f034e33
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Mon Nov 14 22:41:56 2016

[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only. This is
essentially a partial revert of the above patch, but still leaves us in
a much better state than when all ports were created synchronously
(especially since we know no ports are created synchronously during page
load). In the long run, this is something we want to discourage, but
that should be done deliberately, and the original CL wasn't meant to
break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG= 660706 

Review-Url: https://codereview.chromium.org/2481413004
Cr-Commit-Position: refs/heads/master@{#431925}

[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/chrome/browser/extensions/extension_messages_apitest.cc
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/chrome/browser/renderer_host/chrome_extension_message_filter.cc
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/chrome/browser/renderer_host/chrome_extension_message_filter.h
[add] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/chrome/test/data/extensions/api_test/messaging/on_unload/content_script.js
[add] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/chrome/test/data/extensions/api_test/messaging/on_unload/manifest.json
[add] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/chrome/test/data/extensions/api_test/messaging/on_unload/test.js
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/extensions/common/extension_messages.h
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/extensions/renderer/extension_frame_helper.h
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/extensions/renderer/messaging_bindings.cc
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/extensions/renderer/messaging_bindings.h
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/third_party/WebKit/Source/web/WebDocument.cpp
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/third_party/WebKit/public/web/WebDocument.h
[modify] https://crrev.com/69424807aa1ee34516783d9dd206a95e9f034e33/tools/metrics/histograms/histograms.xml

Comment 6 by rdevlin....@chromium.org, Nov 17 2016

Labels: M-55 Merge-Request-55
Status: Fixed (was: Assigned)

Comment 7 by dimu@chromium.org, Nov 17 2016

Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 17 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a

commit 04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Nov 17 19:44:12 2016

[Extensions] Make port creation synchronous in unload handlers

https://crrev.com/dbbe69e7a03b50940d5b7e615c04de531511cac3 made
extension message port initialization asynchronous in order to
address performance concerns around having a synchronous IPC.
Unfortunatley, this broke use cases where extensions use
chrome.runtime.sendMessage() from the unload handler for a page.

Fix this by using synchronous port creation in this case only. This is
essentially a partial revert of the above patch, but still leaves us in
a much better state than when all ports were created synchronously
(especially since we know no ports are created synchronously during page
load). In the long run, this is something we want to discourage, but
that should be done deliberately, and the original CL wasn't meant to
break anything.

Add a test to ensure we don't regress in the future.

Also add metrics for tracking the number of ports created during unload
handlers so that we can determine how common of an occurrence this is,
which can influence our decisions going forward.

BUG= 660706 

Review-Url: https://codereview.chromium.org/2481413004
Cr-Commit-Position: refs/heads/master@{#431925}
(cherry picked from commit 69424807aa1ee34516783d9dd206a95e9f034e33)

Review URL: https://codereview.chromium.org/2514523002 .

Cr-Commit-Position: refs/branch-heads/2883@{#600}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/chrome/browser/extensions/extension_messages_apitest.cc
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/chrome/browser/renderer_host/chrome_extension_message_filter.cc
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/chrome/browser/renderer_host/chrome_extension_message_filter.h
[add] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/chrome/test/data/extensions/api_test/messaging/on_unload/content_script.js
[add] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/chrome/test/data/extensions/api_test/messaging/on_unload/manifest.json
[add] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/chrome/test/data/extensions/api_test/messaging/on_unload/test.js
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/extensions/common/extension_messages.h
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/extensions/renderer/extension_frame_helper.h
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/extensions/renderer/messaging_bindings.cc
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/extensions/renderer/messaging_bindings.h
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/third_party/WebKit/Source/web/WebDocument.cpp
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/third_party/WebKit/public/web/WebDocument.h
[modify] https://crrev.com/04e0af8f5c97dc8e0ca69a9da5324ce9f84f950a/tools/metrics/histograms/histograms.xml

Comment 9 by brajkumar@chromium.org, Nov 21 2016

Labels: TE-Verified-55.0.2883.59 TE-Verified-M55
Verified the issue on Windows-10 using chrome latest M55-55.0.2883.59 by following steps mentioned in the original comment and confirming that chrome.runtime.sendMessage() works for both 'load handler and 'unload' handler as expected. Hence adding TE-Verified label.
660706.PNG
287 KB View Download
►

Sign in to add a comment