(extension) inspector unexpectedly detaches when offline and reloading |
|||||||||||||||
Issue description(I'm reporting the bug here, but Ward Peeters did all the work here investigating the bug and creating a awesome, minimal repro) This is an upstream report of one of our top Lighthouse bugs: https://github.com/GoogleChrome/lighthouse/issues/5798 1. install unpacked extension https://github.com/wardpeet/lh-extension-bug-repro 2. go to any site. http://example.com is fine. 3. open the extension's popup. 4. click its button once to attach 5. click its button again to trigger a reload Problem: the extension is detached from chrome.debugger unexpectedly Expected: the extension stays attached. I was able to bisect this problem to this range: https://chromium.googlesource.com/chromium/src/+log/c1d7ebd614d42d911836802022a050f80af8cad9..91f94dcfb2e328e0e5c1f8c61f46e959bb98a556 The likely suspect is "Extract FrameLoader::CancelProvisionalLoaderForNewNavigation" https://chromium-review.googlesource.com/c/chromium/src/+/1107240 --------- In the wild, this repros in the Lighthouse extension on any page served with `Cache-Control: no-cache`. github.com is one example. Interestingly, this isn't an issue for anything but the chrome extension debugger API. The Audits panel and protocol over websocket are both unaffected. Unfortunately, this commit is in m69. So if there's an extension-side workaround we can implement, that'd be a fine temporary solution. wdyt?
,
Sep 10
I'll be requesting a merge to 69 once this lands as is cleared on Canary. Merge expected to be clean.
,
Sep 10
Applying Desktop + Chrome OS as this is Devtools bug. This change is not yet landed in trunk and we already cut M69 Desktop stable RC for release tomorrow, it would be great if extension-side workaround can be implement as temporary solution.
,
Sep 10
I don't think this will need a respin, we can probably take the fix into next security update.
,
Sep 12
Tentative plan is to respin M69 stable early next week. Pls have safe fix ready to merge by this Friday 1:00 PM PT, 09/14 is this is indeed critical or target fix for M70.
,
Sep 14
Following up about Comment #5 to see if the fix will be ready today? Thanks
,
Sep 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5cc9be06edfbda2ccea9453d74584a949bb1d246 commit 5cc9be06edfbda2ccea9453d74584a949bb1d246 Author: Dmitry Gozman <dgozman@chromium.org> Date: Sat Sep 15 01:12:55 2018 [DevTools] Allow debugger extensions to access error page Bug: 882589 Change-Id: Ie834bfa3e6a14657924d67d179b547bd850072b3 Reviewed-on: https://chromium-review.googlesource.com/1217599 Commit-Queue: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Paul Irish <paulirish@chromium.org> Cr-Commit-Position: refs/heads/master@{#591547} [modify] https://crrev.com/5cc9be06edfbda2ccea9453d74584a949bb1d246/chrome/browser/extensions/api/debugger/debugger_api.cc [modify] https://crrev.com/5cc9be06edfbda2ccea9453d74584a949bb1d246/chrome/browser/extensions/api/debugger/debugger_apitest.cc [modify] https://crrev.com/5cc9be06edfbda2ccea9453d74584a949bb1d246/chrome/test/data/extensions/api_test/debugger/background.js
,
Sep 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b176d2cbcc4abf8596cab1ad349bebc9b3252b3c commit b176d2cbcc4abf8596cab1ad349bebc9b3252b3c Author: Darren Shen <shend@chromium.org> Date: Mon Sep 17 01:53:41 2018 Revert "[DevTools] Allow debugger extensions to access error page" This reverts commit 5cc9be06edfbda2ccea9453d74584a949bb1d246. Reason for revert: crbug.com/884567 Original change's description: > [DevTools] Allow debugger extensions to access error page > > Bug: 882589 > Change-Id: Ie834bfa3e6a14657924d67d179b547bd850072b3 > Reviewed-on: https://chromium-review.googlesource.com/1217599 > Commit-Queue: Dmitry Gozman <dgozman@chromium.org> > Reviewed-by: Devlin <rdevlin.cronin@chromium.org> > Reviewed-by: Paul Irish <paulirish@chromium.org> > Cr-Commit-Position: refs/heads/master@{#591547} TBR=dgozman@chromium.org,nasko@chromium.org,rdevlin.cronin@chromium.org,paulirish@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 882589 Change-Id: I4fbe9d85deaf80661d1506960132e2c4e0d9efd1 Reviewed-on: https://chromium-review.googlesource.com/1226726 Reviewed-by: Darren Shen <shend@chromium.org> Commit-Queue: Darren Shen <shend@chromium.org> Cr-Commit-Position: refs/heads/master@{#591608} [modify] https://crrev.com/b176d2cbcc4abf8596cab1ad349bebc9b3252b3c/chrome/browser/extensions/api/debugger/debugger_api.cc [modify] https://crrev.com/b176d2cbcc4abf8596cab1ad349bebc9b3252b3c/chrome/browser/extensions/api/debugger/debugger_apitest.cc [modify] https://crrev.com/b176d2cbcc4abf8596cab1ad349bebc9b3252b3c/chrome/test/data/extensions/api_test/debugger/background.js
,
Sep 17
The error page isolation is not enabled on M69, so moving this issue to M70.
,
Sep 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00ba04e02c759d6de1df3560da4c5dbc8fa976b0 commit 00ba04e02c759d6de1df3560da4c5dbc8fa976b0 Author: Dmitry Gozman <dgozman@chromium.org> Date: Tue Sep 18 00:39:27 2018 Reland "[DevTools] Allow debugger extensions to access error page" TBR=rdevlin.cronin@chromium.org Bug: 882589 Change-Id: I46f7af5b56dccd7ee73318d5efea6caaa9dbb91b Reviewed-on: https://chromium-review.googlesource.com/1228789 Commit-Queue: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Paul Irish <paulirish@chromium.org> Cr-Commit-Position: refs/heads/master@{#591898} [modify] https://crrev.com/00ba04e02c759d6de1df3560da4c5dbc8fa976b0/chrome/browser/extensions/api/debugger/debugger_api.cc [modify] https://crrev.com/00ba04e02c759d6de1df3560da4c5dbc8fa976b0/chrome/browser/extensions/api/debugger/debugger_apitest.cc [modify] https://crrev.com/00ba04e02c759d6de1df3560da4c5dbc8fa976b0/chrome/test/data/extensions/api_test/debugger/background.js
,
Sep 19
The commit from #c10 did stick, so I am marking this as Fixed. Will request merge to M70 after verifying on Canary.
,
Sep 19
[Auto-generated comment by a script] We noticed that this issue is targeted for M-70; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-70 label, otherwise remove Merge-TBD label. Thanks.
,
Sep 21
,
Sep 21
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 21
,
Sep 24
How does this look in canary?
,
Sep 24
It seems like this is still broken on ToT (thanks, Paul!), but with another symptom - "about:" site instance url. Let me look into this again.
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d0b3f1a84d0c89bd372f6fabde5c317b8bf6642 commit 4d0b3f1a84d0c89bd372f6fabde5c317b8bf6642 Author: Dmitry Gozman <dgozman@chromium.org> Date: Tue Sep 25 16:39:10 2018 [DevTools] Allow extensions to debug about:blank When navigation to "about:blank" gets a new render process, we get site instance url equal to "about:". In this case our standard check for url permission fails. Usually about:blank does not go to a separate process, but there are some rare cases: - navigating away from isolated error page forces a new site instance; - navigating after crash creates a new renderer, since there is no existing one to reuse. Bug: 882589 Change-Id: I076ab544ca2360e7e4c13b58475c08e0be627593 Reviewed-on: https://chromium-review.googlesource.com/1240865 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#593966} [modify] https://crrev.com/4d0b3f1a84d0c89bd372f6fabde5c317b8bf6642/chrome/browser/extensions/api/debugger/debugger_api.cc [modify] https://crrev.com/4d0b3f1a84d0c89bd372f6fabde5c317b8bf6642/chrome/test/data/extensions/api_test/debugger/background.js
,
Sep 27
The fix from #c18 works in canary, requesting a merge to M70.
,
Sep 27
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 28
,
Oct 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/61501d236c69c19e2b0ed4eb7b3da5faf8791c0c Commit: 61501d236c69c19e2b0ed4eb7b3da5faf8791c0c Author: dgozman@chromium.org Commiter: dgozman@chromium.org Date: 2018-10-01 16:52:46 +0000 UTC Reland "[DevTools] Allow debugger extensions to access error page" TBR=dgozman@chromium.org, rdevlin.cronin@chromium.org (cherry picked from commit 00ba04e02c759d6de1df3560da4c5dbc8fa976b0) Bug: 882589 Change-Id: I46f7af5b56dccd7ee73318d5efea6caaa9dbb91b Reviewed-on: https://chromium-review.googlesource.com/1228789 Commit-Queue: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Paul Irish <paulirish@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#591898} Reviewed-on: https://chromium-review.googlesource.com/1254752 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#774} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/61501d236c69c19e2b0ed4eb7b3da5faf8791c0c commit 61501d236c69c19e2b0ed4eb7b3da5faf8791c0c Author: Dmitry Gozman <dgozman@chromium.org> Date: Mon Oct 01 16:52:46 2018 Reland "[DevTools] Allow debugger extensions to access error page" TBR=dgozman@chromium.org, rdevlin.cronin@chromium.org (cherry picked from commit 00ba04e02c759d6de1df3560da4c5dbc8fa976b0) Bug: 882589 Change-Id: I46f7af5b56dccd7ee73318d5efea6caaa9dbb91b Reviewed-on: https://chromium-review.googlesource.com/1228789 Commit-Queue: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Paul Irish <paulirish@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#591898} Reviewed-on: https://chromium-review.googlesource.com/1254752 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#774} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/61501d236c69c19e2b0ed4eb7b3da5faf8791c0c/chrome/browser/extensions/api/debugger/debugger_api.cc
,
Oct 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ff5f9c932b91c4abc92f2d110df75918eb52fd4 commit 4ff5f9c932b91c4abc92f2d110df75918eb52fd4 Author: Dmitry Gozman <dgozman@chromium.org> Date: Mon Oct 01 16:58:13 2018 [DevTools] Allow extensions to debug about:blank When navigation to "about:blank" gets a new render process, we get site instance url equal to "about:". In this case our standard check for url permission fails. Usually about:blank does not go to a separate process, but there are some rare cases: - navigating away from isolated error page forces a new site instance; - navigating after crash creates a new renderer, since there is no existing one to reuse. TBR=dgozman@chromium.org, rdevlin.cronin@chromium.org (cherry picked from commit 4d0b3f1a84d0c89bd372f6fabde5c317b8bf6642) Bug: 882589 Change-Id: I076ab544ca2360e7e4c13b58475c08e0be627593 Reviewed-on: https://chromium-review.googlesource.com/1240865 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Dmitry Gozman <dgozman@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#593966} Reviewed-on: https://chromium-review.googlesource.com/1254803 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#775} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/4ff5f9c932b91c4abc92f2d110df75918eb52fd4/chrome/browser/extensions/api/debugger/debugger_api.cc
,
Oct 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ff5f9c932b91c4abc92f2d110df75918eb52fd4 Commit: 4ff5f9c932b91c4abc92f2d110df75918eb52fd4 Author: dgozman@chromium.org Commiter: dgozman@chromium.org Date: 2018-10-01 16:58:13 +0000 UTC [DevTools] Allow extensions to debug about:blank When navigation to "about:blank" gets a new render process, we get site instance url equal to "about:". In this case our standard check for url permission fails. Usually about:blank does not go to a separate process, but there are some rare cases: - navigating away from isolated error page forces a new site instance; - navigating after crash creates a new renderer, since there is no existing one to reuse. TBR=dgozman@chromium.org, rdevlin.cronin@chromium.org (cherry picked from commit 4d0b3f1a84d0c89bd372f6fabde5c317b8bf6642) Bug: 882589 Change-Id: I076ab544ca2360e7e4c13b58475c08e0be627593 Reviewed-on: https://chromium-review.googlesource.com/1240865 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Dmitry Gozman <dgozman@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#593966} Reviewed-on: https://chromium-review.googlesource.com/1254803 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#775} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 3
Able to reproduce the issue on win-10 using chrome build without fix. Verified the fix on Win-10, mac 10.13.3 and ubuntu 17.10 using Chrome version #70.0.3538.45 as per the comment#0 Attaching screen cast for reference. Observed that (extension) inspector did not detach when offline and reloading. Hence, the fix is working as expected. Adding the verified label. Thanks...!! |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by paulir...@chromium.org
, Sep 10Status: Started (was: Assigned)