1211970 - Muted errors in workers are not correctly reported to the console

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Boris Zbarsky [: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?

Comment 3

[Andrea Marchesini [:baku]]

> 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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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().

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch

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.

Comment 7

[Boris Zbarsky [: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

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Care to just nix the unused first arg of that constructor?

Ah, you already filed bug 1214300, ok.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: worker4.patch

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f4be1ae066

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5f4be1ae066

Comment 12

[Andrea Marchesini [:baku]]

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.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

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?

Comment 14

[Boris Zbarsky [:bzbarsky]]

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...

Comment 15

[Boris Zbarsky [:bzbarsky]]

I guess no one ever set tracking flags on this bug either.... <sigh>.

We really should not have backported bug 1160890, imo.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 18

[Al Billings [:abillings - ex-MoCo]]

I'd rather back out than take more changes.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out bug 1160890 on 42. 

I'll get this on aurora in a bit.

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Andrea Marchesini [:baku]]

Attachment: patch for m-a

Comment 23

[Carsten Book [:Tomcat]]

(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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

baku says that the patch we landed in comment 23 also addresses the issues in bug 1211967. So now we should be done.  Huzzah!

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 28

[Andrea Marchesini [:baku]]

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.