New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 285976



Sign in to add a comment
link

Issue 359423: Propagate error/exception reports from SW to browser

Reported by kinuko@chromium.org, Apr 3 2014 Project Member

Issue description

We also need another task to dispatch error events from browser to SW object in docs, but this issue doesn't cover it. (Until we have the other part we can probably also show errors via DVLOG's or in serviceworker-internals on the browser side for those errors)

1. For regular errors/exceptions in SW code:

* In Blink worker errors/exceptions are reported via WorkerReportingProxy interface, which is implemented by ServiceWorkerGlobalScopeProxy in SW case.
* In SWGlobalScopeProxy we can call out to chromium via SWGlobalScopeClient (m_client), which is implemented by EmbeddedWorkerContextClient in chromium.
* You can basically report the error back to browser by implementing EmbeddedWorkerContextClient::reportException(), to send an IPC to browser.

2. For rejected promise which is handled in C++ (like installEvent's waitUntil() or fetchEvent's reportWith()):

* We probalby want to propagate these errors too, the entry points for them are WaitUntilObserver::reportError() and RespondWithObserver::responseWasRejected(). You should be able to get Error object from ScriptValue to get necessary error reports.
* From there I think we can access WorkerReportingProxy (these observers have a pointer to ExecutionContext, which can be casted to WorkerGlobalScope which has a pointer to WorkerThread which has WorkerReportingProxy)


Work 1. needs code in chromium, while work 2. needs code in Blink. Probably 1. has higher priority and easier to start with.

Fyi, for 1. we used to have similar code for old workers though we deprecated the code for regular workers today: https://codereview.chromium.org/33753003 is the one that removed the corresponding code, you may want to take a quick look for reference
 

Comment 1 by kinuko@chromium.org, Apr 3 2014

Blocking: chromium:285976

Comment 2 by kinuko@chromium.org, Apr 3 2014

Status: Assigned

Comment 3 by nhiroki@chromium.org, Apr 3 2014

Status: Started
Thank you for the instruction! I'll take up work 1. first.

Comment 5 by nhiroki@chromium.org, Apr 9 2014

Cc: horo@chromium.org

Comment 6 by nhiroki@chromium.org, Apr 30 2014

Cc: dominicc@chromium.org
(Status update)

About work 1, it was already done by r262606 (c#4)
About work 2, I just started working on it and have a half-baked patch. I'll upload it soon.

Comment 7 by kinuko@chromium.org, Apr 30 2014

Labels: -Pri-1 Pri-2
Since work 1 is landed let me change P2 -> P1, we don't have onerror code yet, and finishing script persistence will have a bigger impact.

Comment 8 by dominicc@chromium.org, May 7 2014

Status: Available
This is nice to have for 37 but not critical; not sure about reject cases for install. It's more important for fetch.

Comment 9 by nhiroki@chromium.org, Sep 26 2014

Cc: -serviceworker-reviews@chromium.org nhiroki@chromium.org
Owner: ----

Comment 10 by mayurk...@samsung.com, Sep 26 2014

If no one else is working on this, I would like to take this up.

Comment 11 by dominicc@google.com, Sep 29 2014

Owner: dominicc@chromium.org
Status: Started
Assigning this to me as a proxy for mayurk.vk.

Mayurk: This is all yours--let me know if you need anything from me!

Comment 12 by nhiroki@chromium.org, Sep 29 2014

Labels: M-40

Comment 13 by mayurk...@samsung.com, Sep 29 2014

Checking the 2nd work item. The blink side of propagating SW exceptions.

Comment 14 by dominicc@google.com, Oct 8 2014

Cc: mayurk...@samsung.com
Any update on this?

Comment 15 by dominicc@google.com, Oct 8 2014

 Issue 417554  has been merged into this issue.

Comment 16 Deleted

Comment 17 by mayurk...@samsung.com, Oct 9 2014

Got the requirement about the Error part, working on the final solution for pt 2.

Comment 18 by mayurk...@samsung.com, Oct 9 2014

Hi dominicc,
I added code to pass on the error information to the WorkerReportingProxy(https://codereview.chromium.org/641083002/).
But when am checking this example(attached), it does not show any information on the console. 
Is it expected to show it on the console as well?
Thanks.
index.html
1.3 KB View Download
service-worker.js
484 bytes View Download

Comment 19 by dominicc@google.com, Oct 20 2014

Additionally, we should make these messages show up in adb logcat when the Service Worker is running on a mobile device. I think that will Just Work when we add this plumbing but we should confirm it.

Comment 20 by dominicc@chromium.org, Oct 23 2014

Mayurk, sorry for the slow response. You need to pass command line flags to Chrome to turn on logging, see http://www.chromium.org/for-testers/enable-logging . You want:

--enable-logging --v=1

I'm applying your patch locally now to test it.

Comment 21 by dominicc@chromium.org, Oct 24 2014

Cc: -mayurk...@samsung.com aandrey@chromium.org
+aandrey--I'm thinking of changing the heuristics in V8Initialization so that rejection of some specific Promises is always reported. The current "has handler" type heuristics don't work for Service Worker, because the Promises have our native C++ handler.

I have a hacky local patch now that adds virtuals to ExecutionContext so it can set the policy about what gets reported. But now I'm thinking of just tagging "interesting" Promises with a hidden property or something, and the Promise reporting stuff can check for that.

If you have ideas/feedback let me know.

Comment 22 by aandrey@chromium.org, Oct 24 2014

@dominicc, if I understand you right, you'd like to be able to do a special promise.catch(function() {}), that would not alter the handled-unhandled state of the original promise, right? With ordinary exceptions it'd be a rethrow from try-catch.

Maybe just "rethrowing" the promise rejection will help you?

promise.catch(function(e) {
  // do your stuff in native C++ callback
  // ...
  // rethrow promise rejection to get it logged to DevTools console
  return Promise.reject(e);
});

Comment 23 by dominicc@chromium.org, Oct 27 2014

Neat solution. I'll implement that. Thanks!

Comment 24 by dominicc@chromium.org, Oct 30 2014

Status: Available
OK, it's a couple of lines to hook up rejections in this way, but unfortunately this needs a browsertest (!) because other error reporting infrastructure is not hooked up. So maybe it is better to implement onerror first and use that for testing.

I'm also not seeing good file/line number information for Promises rejected in Service Workers; we should look at that.

Putting this back on the shelf to pick up a P1 instead.

Comment 25 by aandrey@chromium.org, Oct 30 2014

> I'm also not seeing good file/line number information for Promises rejected in Service Workers

This is for all promises, not only in SW. I'm working on that, see  bug 427954 .

Comment 26 by dominicc@chromium.org, Nov 10 2014

Labels: -M-40 M-41

Comment 27 by falken@chromium.org, Nov 28 2014

This bug has gotten long. dominicc@, could you summarize what work remains to be done?

Comment 28 by dominicc@chromium.org, Dec 5 2014

I think we should:

- Implement the suggestion in Comment 22 in our CallbackPromiseAdapters.
- Write a LayoutTest using onerror that verifies this "shows up" as an error.
- That's it! The rising tide of better Promise debugging will raise our boat along with it after that.

Comment 29 by falken@chromium.org, Jan 13 2015

Labels: -M-41 M-42

Comment 30 by kenjibaheux@chromium.org, Jan 15 2015

Cc: yhirano@chromium.org
 Issue 448281  has been merged into this issue.

Comment 31 by amineer@chromium.org, Mar 3 2015

Labels: -M-42 M-43 MovedFrom-42
Status: Assigned
[AUTO] Moving all non essential bugs to the next Milestone.  (This decision is based on the labels attached to your ticket.)


Ref: https://sites.google.com/a/chromium.org/dev/developers/ticket-milestone-punting-1

Comment 32 by dominicc@chromium.org, Mar 10 2015

Status: Started
Actively working on this again.

Comment 33 by dominicc@chromium.org, Mar 11 2015

Quick update: Have this logging in the Service Worker console; that's easy. The problem is testing it. That's hard.

It would be nice if testRunner was injected in worker environments as well as page environments for layout tests.

Comment 34 by kinuko@chromium.org, Apr 4 2015

#33: horo@ says he (probably) knows how to write a test for this, if you have a patch you can hand over to horo@ if you want.

Comment 35 by kenjibaheux@chromium.org, Apr 7 2015

Labels: -M-43 M-44
Owner: ----
Status: Available

Comment 36 by kinuko@chromium.org, Apr 7 2015

Owner: ksakamoto@chromium.org

Comment 37 by kenjibaheux@chromium.org, Apr 7 2015

Labels: Release-notes-M44
Status: Assigned

Comment 38 by bugdroid1@chromium.org, Apr 15 2015

Project Member
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=193762

------------------------------------------------------------------
r193762 | ksakamoto@chromium.org | 2015-04-15T08:55:40.729766Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/serviceworkers/WaitUntilObserver.cpp?r1=193762&r2=193761&pathrev=193762
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/serviceworkers/RespondWithObserver.cpp?r1=193762&r2=193761&pathrev=193762
   M http://src.chromium.org/viewvc/blink/trunk/Source/bindings/core/v8/ScriptPromise.h?r1=193762&r2=193761&pathrev=193762

waitUntil() and respondWith() should propagate error/exception reports

This patch makes WaitUntilObserver and RespondWithObserver "rethrow"
errors when rejection handler is called. As a result, messages are
reported in worker console.

BUG= 359423 
TEST=https://codereview.chromium.org/1081953004/

Review URL: https://codereview.chromium.org/1078603002
-----------------------------------------------------------------

Comment 39 by bugdroid1@chromium.org, Apr 16 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f64887fa4067a08766179a313b623115b1e8f07

commit 7f64887fa4067a08766179a313b623115b1e8f07
Author: ksakamoto <ksakamoto@chromium.org>
Date: Thu Apr 16 02:53:38 2015

Add tests for console messages of waitUntil() and respondWith()

This adds browser tests that verify console message logged when a
promise passed to waitUntil() or respondWith() is rejected.

These tests are here because console outputs from SWs aren't testable
with layout tests ( crbug.com/456708 ).

BUG= 359423 

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

Cr-Commit-Position: refs/heads/master@{#325383}

[modify] http://crrev.com/7f64887fa4067a08766179a313b623115b1e8f07/content/browser/service_worker/service_worker_browsertest.cc
[add] http://crrev.com/7f64887fa4067a08766179a313b623115b1e8f07/content/test/data/service_worker/fetch_event_rejected.js
[modify] http://crrev.com/7f64887fa4067a08766179a313b623115b1e8f07/content/test/data/service_worker/worker_install_rejected.js

Comment 40 by ksakamoto@chromium.org, Apr 16 2015

Status: Fixed

Sign in to add a comment