New issue
Advanced search Search tips

Issue 652937 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

DevTools are unable to start a shared worker

Project Member Reported by caseq@chromium.org, Oct 5 2016

Issue description

1. Ensure using a build with debug_devtools=false (any production build will also do)
2. Open DevTools
3. Switch to Timeline panel
4. Do a short recording
5. Save to file

Observe the saved file is zero bytes in length. Also, open DevTools-on-DevTools and evaluate WebInspector.TempFile._storageCleanerPromise -- observe promise is in "pending" state.

This is because the shared worker DevTools tried to start here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/bindings/TempFile.js?rcl=0&l=421 quietly fails.

This is in turn because of the shared worker's main script loader failing to load the script from URLDataSource due to a check here:
https://cs.chromium.org/chromium/src/content/browser/webui/url_data_manager_backend.cc?rcl=0&l=761

Apparently, we now expect every request to the URLDataSource come from a WebContents (as per https://cs.chromium.org/chromium/src/content/public/browser/url_data_source.h?rcl=0&l=59, "If this method is called on the UI thread, then it's guaranteed that wc_getter will return a non-null WebContents."), and hence it fails for the shared workers.

This has be regressed by https://chromium.googlesource.com/chromium/src/+/edcd8b01496d777037b3752d82ac64a16b6cedd0

 

Comment 1 by jam@chromium.org, Oct 5 2016

Status: Started (was: Assigned)
oops, sorry. I'll take a look.

Do we have infrastructure for how to write a test for this? Any pointers would be appreciated.

Comment 2 by jam@chromium.org, Oct 5 2016

btw the documentation went out of date between ps1 & ps2: WebContents has to be valid only if the WC getter is not null. so in this case we can pass a null WC getter.

Comment 3 by caseq@chromium.org, Oct 5 2016

> Do we have infrastructure for how to write a test for this? Any pointers would be appreciated

I can take care of the test, I think this should be easy to as a DevToolsSanityTest.

Comment 4 by jam@chromium.org, Oct 5 2016

Ok thank you.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 5 2016

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

commit f72ebb00ab29f5ed639d24dd24b44f4ea6201a42
Author: jam <jam@chromium.org>
Date: Wed Oct 05 17:33:30 2016

Fix devtools unable to start a shared workers.

This regressed in r419646 because it changed a check for the request's process being alive to instead check if the WebContents is alive. The latter isn't true for worker-initiated requests.

Since URLDataManagerBackend's behavior now changed back to what it used to do, I've also updated the data sources that were modified in the same cl to bring back WebContents null checks.

BUG= 652937 

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

[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/chrome/browser/search/iframe_source.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/chrome/browser/ui/webui/app_launcher_page_ui.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/chrome/browser/ui/webui/ntp/new_tab_ui.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/components/dom_distiller/content/browser/dom_distiller_viewer_source.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/content/browser/webui/url_data_manager_backend.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/content/browser/webui/url_data_manager_backend.h
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/content/public/browser/url_data_source.h

Comment 6 by jam@chromium.org, Oct 5 2016

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 5 2016

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

commit cb5ed93a66692ba25f8fe57559a4b8e67401c165
Author: caseq <caseq@chromium.org>
Date: Wed Oct 05 21:37:01 2016

DevTools: add a test for storage cleanup worker

It did not have any test coverage in the browser and it's critical
for DevTools to be able to save things to files.

BUG= 652937 

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

[modify] https://crrev.com/cb5ed93a66692ba25f8fe57559a4b8e67401c165/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/cb5ed93a66692ba25f8fe57559a4b8e67401c165/third_party/WebKit/Source/devtools/front_end/Tests.js

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42

commit f72ebb00ab29f5ed639d24dd24b44f4ea6201a42
Author: jam <jam@chromium.org>
Date: Wed Oct 05 17:33:30 2016

Fix devtools unable to start a shared workers.

This regressed in r419646 because it changed a check for the request's process being alive to instead check if the WebContents is alive. The latter isn't true for worker-initiated requests.

Since URLDataManagerBackend's behavior now changed back to what it used to do, I've also updated the data sources that were modified in the same cl to bring back WebContents null checks.

BUG= 652937 

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

[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/chrome/browser/search/iframe_source.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/chrome/browser/ui/webui/app_launcher_page_ui.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/chrome/browser/ui/webui/ntp/new_tab_ui.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/components/dom_distiller/content/browser/dom_distiller_viewer_source.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/content/browser/webui/url_data_manager_backend.cc
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/content/browser/webui/url_data_manager_backend.h
[modify] https://crrev.com/f72ebb00ab29f5ed639d24dd24b44f4ea6201a42/content/public/browser/url_data_source.h

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 27 2016

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

commit cb5ed93a66692ba25f8fe57559a4b8e67401c165
Author: caseq <caseq@chromium.org>
Date: Wed Oct 05 21:37:01 2016

DevTools: add a test for storage cleanup worker

It did not have any test coverage in the browser and it's critical
for DevTools to be able to save things to files.

BUG= 652937 

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

[modify] https://crrev.com/cb5ed93a66692ba25f8fe57559a4b8e67401c165/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/cb5ed93a66692ba25f8fe57559a4b8e67401c165/third_party/WebKit/Source/devtools/front_end/Tests.js

Comment 10 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840

Sign in to add a comment