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

Propagate error/exception reports from SW to browser

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

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

 
Blocking: chromium:285976
Status: Assigned
Status: Started
Thank you for the instruction! I'll take up work 1. first. 
Cc: horo@chromium.org
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.
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.
Cc: -serviceworker-reviews@chromium.org nhiroki@chromium.org
Owner: ----
If no one else is working on this, I would like to take this up.
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!
Labels: M-40
Checking the 2nd work item. The blink side of propagating SW exceptions.
Cc: mayurk...@samsung.com
Any update on this?
 Issue 417554  has been merged into this issue.

Comment 16 Deleted

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

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
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.
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.
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.
@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);
});

Neat solution. I'll implement that. Thanks!
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.
> 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 .
Labels: -M-40 M-41
This bug has gotten long. dominicc@, could you summarize what work remains to be done?
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.
Labels: -M-41 M-42
Cc: yhirano@chromium.org
 Issue 448281  has been merged into this issue.
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
Status: Started
Actively working on this again.
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.
#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.
Labels: -M-43 M-44
Owner: ----
Status: Available
Owner: ksakamoto@chromium.org
Labels: Release-notes-M44
Status: Assigned
Project Member

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

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
-----------------------------------------------------------------
Project Member

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

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

Status: Fixed

Sign in to add a comment