Muted errors in workers are not correctly reported to the console

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed, firefox44 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

4 years ago
No description provided.
Assignee

Comment 1

4 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8673211 - Flags: review?(bzbarsky)
Comment on attachment 8673211 [details] [diff] [review]
patch

Why can't this use whatever infrastructure we normally use for reporting uncaught exceptions on workers?  It seems odd that we don't have something to handle this already....

Looking around a bit, we have existing stuff similar to this in RejectedCallback in ServiceWorkerPrivate, that's much simpler.  Is that just because it doesn't need to get a window id?  Something else?
Flags: needinfo?(amarchesini)
Assignee

Comment 3

4 years ago
> Looking around a bit, we have existing stuff similar to this in
> RejectedCallback in ServiceWorkerPrivate, that's much simpler.  Is that just
> because it doesn't need to get a window id?  Something else?

Exactly this is the reason. Without window ID this error report will not be shown in the web console.
Flags: needinfo?(amarchesini)
Does WindowID() on the WorkerPrivate not do what we want already?  If not, WorkerPrivate::GetParent() ok to call on the worker thread?  If so, is there a reason we can't just walk up the GetParent() chain and then call WindowID() on the root workerprivate, all on the worker thread?
A quick look at WorkerPrivate::GetLoadInfo suggests that WindowID() should in fact do the right thing for us.  If aParent it's set to the parent's WindowID(), and otherwise if aWindow it's set to that window's WindowID().
Assignee

Updated

4 years ago
Assignee: nobody → amarchesini
Assignee

Comment 6

4 years ago
Posted patch patch (obsolete) — Splinter Review
Yes, you are right. I was just following a pattern. I'll file a follow up to remove this way to retrieve the windowID in other worker runnables.
Attachment #8673211 - Attachment is obsolete: true
Attachment #8673211 - Flags: review?(bzbarsky)
Attachment #8673240 - Flags: review?(bzbarsky)
Comment on attachment 8673240 [details] [diff] [review]
patch

Yeah, this is much better.  ;)

You should probably JS_ClearPendingException(aCx) if !report.init().

>+    new AsyncErrorReporter(CycleCollectedJSRuntime::Get()->Runtime(), xpcReport);

Care to just nix the unused first arg of that constructor?  That will stop me from worrying about whether the "right" runtime is being used.  ;)

r=me
Attachment #8673240 - Flags: review?(bzbarsky) → review+
> Care to just nix the unused first arg of that constructor?

Ah, you already filed bug 1214300, ok.
Assignee

Comment 9

4 years ago
Posted patch worker4.patchSplinter Review
Attachment #8673240 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c5f4be1ae066
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee

Comment 12

4 years ago
Comment on attachment 8674192 [details] [diff] [review]
worker4.patch

[Feature/regressing bug #]: Workers
[User impact if declined]: a cross-origin imported script errors are not correctly reported to the console.
[Describe test coverage new/current, TreeHerder]: none.
[Risks and why]: low risks. We just dispatch a runnable.
[String/UUID change made/needed]: none.
Attachment #8674192 - Flags: approval-mozilla-beta?
Attachment #8674192 - Flags: approval-mozilla-aurora?
To state the obvious, we are going to build 42 rc today. I am unhappy to see that the patch landed a week ago and could have landed in beta 8.

> [Feature/regressing bug #]: Workers
Could you be a bit more specific here?

> [Describe test coverage new/current, TreeHerder]: none.
You haven't done any testing?

> We just dispatch a runnable.
Why this is low risk?
Flags: needinfo?(amarchesini)
Note that backporting this requires backporting bug 1214300 as well.  But that should be quite safe: as long as the result compiles it's fine.  If it does not compile, this patch is simple to modify to work around needing the patch from bug 1214300.

> I am unhappy to see that the patch landed a week ago and could have landed in beta 8.

While true, and the reason I tried pinging you about this stuff in bug 1160890 a while ago, how is Andrea supposed to know when the beta and rc dates are?  https://wiki.mozilla.org/RapidRelease/Calendar doesn't have them, and they were not mentioned in bug 1160890.  And I don't personally have the bandwidth tho shepherd in every bug I review.... I thought that was what release drivers were supposed to keep track of: whether regressions from backported patches had also gotten properly backported?  Is that not the case?  I so, there's been some miscommunication, and I should have pushed back _much_ harder on backporting bug 1160890 given the timeframes and known regressions even before it landed.

> Could you be a bit more specific here?

Bug 1160890 regressed this, as was expected when it landed and known when it was backported.

> You haven't done any testing?

Manual testing has been done.  There is no automated testing for this yet, though we should add some.

> Why this is low risk?

What the patch does is to replace a "clear the exception" call with a "grab the exception, stash its information in a runnable, clear it, post the runnable to the main thread to log to console".  There are plenty of other codepaths in our tree that exercise the "stash its information in a runnable" and "log from the stashed information" bits, so thos are well tested.  Posting runnables is pretty well-tested too.  So this patch should in fact be quite low-risk.

Maybe the right answer is to back bug 1160890 out of 42 instead...
Flags: needinfo?(sledru)
I guess no one ever set tracking flags on this bug either.... <sigh>.

We really should not have backported bug 1160890, imo.
Yes, without the tracking requests or uplift request, I didn't have a way to find out about this bug.

Anyway, just like the schedule of release (hint: https://calendar.google.com/calendar/embed?src=bW96aWxsYS5jb21fZGJxODRhbnI5aTh0Y25taGFiYXRzdHY1Y29AZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ ), this is not the topic.

I would be happy to backout bug 1160890. Al, would you be ok or it is too late?
If not, we would have to take the two other patches, adding more risk on the release.
Flags: needinfo?(sledru) → needinfo?(abillings)
> I didn't have a way to find out about this bug.

Uh...  I told you about this bug and bug 1211967 in bug 1160890 comment 68.  I probably should have added the tracking flags then too.  Or you should have.  Or Andrea should have.  Or all of us.

Realistically, we should auto-add tracking flags to any regression from a backported bug...
I'd rather back out than take more changes.
Flags: needinfo?(abillings)
Assignee

Updated

4 years ago
Flags: needinfo?(amarchesini)
Comment on attachment 8674192 [details] [diff] [review]
worker4.patch

Let's uplift this to aurora (43). I don't think Sylvestre would like it in beta for the moment as 42 is going to RC today.
Attachment #8674192 - Flags: approval-mozilla-beta?
Attachment #8674192 - Flags: approval-mozilla-beta-
Attachment #8674192 - Flags: approval-mozilla-aurora?
Attachment #8674192 - Flags: approval-mozilla-aurora+
Backed out bug 1160890 on 42. 

I'll get this on aurora in a bit.
backed this out from aurora for bustage like  https://treeherder.mozilla.org/logviewer.html#?job_id=1390896&repo=mozilla-aurora from this or the other patch
Flags: needinfo?(amarchesini)
Assignee

Comment 22

4 years ago
Posted file patch for m-a
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #22)
> Created attachment 8679509 [details]
> patch for m-a

landed as https://hg.mozilla.org/releases/mozilla-aurora/rev/2b607d4dccef
OK. Summing this up:  
bug 1160890 - original issue. landed on aurora. comment 71 mentions bug 1211970 (this bug) and bug 1211967.
bug 1214300 - a new patch just landed (comment 23) which has the necessary change from here.   
bug 1211967 - patch landed on aurora, then was backed out. So looks like we still need to figure out why that's broken.
baku says that the patch we landed in comment 23 also addresses the issues in bug 1211967. So now we should be done.  Huzzah!
(In reply to Carsten Book [:Tomcat] from comment #23)

That somehow ended up not being the correct patch that got re-landed.

Backed out the bad one and re-relanded the correct one:
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d9263a7c00b1
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b0f5fa1535dd
> So looks like we still need to figure out why that's broken.

It's not broken.  It was landed in a single push with a "broken" version of this bug, and both were then backed out together.  It just needs to be relanded.

> baku says that the patch we landed in comment 23 also addresses the issues in
> bug 1211967.

It does?  I don't see how.
Flags: needinfo?(amarchesini)
Assignee

Comment 28

4 years ago
What I said is that bug 1214300 is not landed on m-a. The original patch for this bug needs that as dependence. In order to fix this issue, I just posted a new patch that doesn't require bug 1214300 to be in m-a. We definitely still need patch for 1211967.
So please, land on m-a this and bug 1211967.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.