Closed Bug 1150959 Opened 9 years ago Closed 9 years ago

Report sw js errors more aggressively to js console

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: dougt, Assigned: nsm)

References

Details

Attachments

(1 file)

this.addEventListener("activate", function(evt) {
    console.log("SW onactivate");
    if (clients.claim)
        evt.waitUntil(clients.claim());
});


Since we don't support claim right now, this code silently fails.  What we should try to do is log these failures to make developers lives easier.
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment on attachment 8591089 [details] [diff] [review]
Log rejected Promise error message to browser console

Review of attachment 8591089 [details] [diff] [review]:
-----------------------------------------------------------------

it looks good to me, but I prefer to have a review from somebody with more JS API knowledge than what I have.
Attachment #8591089 - Flags: review?(amarchesini) → feedback+
Comment on attachment 8591089 [details] [diff] [review]
Log rejected Promise error message to browser console

Bobby could you review jsapi usage.
Attachment #8591089 - Flags: review?(bobbyholley)
Comment on attachment 8591089 [details] [diff] [review]
Log rejected Promise error message to browser console

Review of attachment 8591089 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +1074,5 @@
> +    JS::Rooted<JSObject*> obj(aCx, workerPrivate->GlobalScope()->GetWrapper());
> +    JS::Rooted<JS::Value> val(aCx, aValue);
> +    JS::ExposeValueToActiveJS(val);
> +
> +    JSAutoCompartment ac(aCx, obj);

Shouldn't we already be in this compartment? Can we jut assert it, and drop the subsequent wrap? There's only one global on workers, right?

@@ +1082,5 @@
> +    }
> +
> +    js::ErrorReport report(aCx);
> +    if (NS_WARN_IF(!report.init(aCx, val))) {
> +      JS_ClearPendingException(aCx);

This pattern is broken - it might limp along, but the best way is to pass an AutoJSAPI& instead of a JSContext*, for which the caller calls TakeOwnershipOfErrorReporting. That might be a bit much to do in this bug though.

@@ +1087,5 @@
> +      return;
> +    }
> +
> +    nsRefPtr<xpc::ErrorReport> xpcReport = new xpc::ErrorReport();
> +    xpcReport->Init(report.report(), report.message(), false /* isChrome */, 0 /* window id */);

I'm used to styling this as |/* aArgumentName = */ value|, but maybe this is another convention that I'm not familiar with.
Attachment #8591089 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/de714e5d0a13
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.