Closed
Bug 812249
Opened 12 years ago
Closed 12 years ago
When "javascript.options.strict" is enabled related warnings/errors trigger worker.onerror handler.
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: jlal, Assigned: baku)
Details
Attachments
(1 file, 3 obsolete files)
|
9.97 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
An example is when referencing an undefined property. The worker's onerror handler will fire. The oddity is that this not an error in the main thread only in workers. For whatever reason we have this setting enabled in gaia by default in some modes and some of our code has these references which cause the .onerror handler to fire in situations that are not actually errors. According to jorendorff this is likely a bug in the worker implementation. I would expect this error to go to the web console instead of onerror.
|
||
Comment 1•12 years ago
|
||
In dom/workers/WorkerPrivate.cpp, use JSREPORT_IS_WARNING(report->flags) to detect when it's just a warning. (It's defined in jsapi.h.)
Andrea, any interest in fixing this?
|
Assignee | |
Updated•12 years ago
|
Assignee: nobody → amarchesini
|
|
Assignee | |
Comment 3•12 years ago
|
||
Attachment #683613 -
Flags: review?(bent.mozilla)
Comment on attachment 683613 [details] [diff] [review] patch Review of attachment 683613 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +982,5 @@ > { > + // The warning messages go to the error console: > + if (JSREPORT_IS_WARNING(aFlags)) { > + return ReportErrorConsole(aMessage, aFilename, aLine, aLineNumber, > + aColumnNumber, aFlags, aInnerWindowId); If we're not on the main thread already (which we aren't, since this is a warning happening on the worker script) then aInnerWindowId is going to be 0. We can only get that on the main thread. So this will pass the wrong id to the console. Can we just follow the normal flow (bouncing this error message runnable to each parent until we get to the main thread) and simply not fire error events if the error is a warning? If that's too hard (I hope not) then I guess we could try to grab the innerwindowid when the worker is created...
|
|
Assignee | |
Comment 5•12 years ago
|
||
Is this what you meant? Here we just fire the event when the error is not a warning. Then we write on console only if we are on the main thread (or at least if the aWorkerPrivate is null).
Attachment #683613 -
Attachment is obsolete: true
Attachment #683613 -
Flags: review?(bent.mozilla)
Attachment #683655 -
Flags: review?(bent.mozilla)
|
|
Assignee | |
Comment 6•12 years ago
|
||
The patch seems moving code, but what I just did in an indentation...
|
||
Comment 7•12 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #6) > The patch seems moving code, but what I just did in an indentation... diff -w will make the review easier.
Comment on attachment 683655 [details] [diff] [review] patch b Review of attachment 683655 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good, assuming it works! It needs a test though before I can r+ it. And I think the test should check one level deep (main thread -> worker has warning) and then two levels deep (main thread -> worker -> worker has warning). ::: dom/workers/WorkerPrivate.cpp @@ +998,5 @@ > if (!filename) { > return false; > } > > + // We create an 'error' event only of error reports. Nit: This comment should say something like "We should not fire error events for warnings but instead make sure that they show up in the error console."
|
|
Assignee | |
Comment 9•12 years ago
|
||
A test has been added
Attachment #683655 -
Attachment is obsolete: true
Attachment #683655 -
Flags: review?(bent.mozilla)
Attachment #688306 -
Flags: review?(bent.mozilla)
Comment on attachment 688306 [details] [diff] [review] patch c Review of attachment 688306 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks! ::: dom/workers/test/test_errorwarning.html @@ +4,5 @@ > +--> > +<!DOCTYPE HTML> > +<html> > +<!-- > +Tests of DOM Worker transferable objects Nit: copypasta
Attachment #688306 -
Flags: review?(bent.mozilla) → review+
|
|
Assignee | |
Comment 11•12 years ago
|
||
Attachment #688306 -
Attachment is obsolete: true
Attachment #688504 -
Flags: review+
|
|
Assignee | |
Comment 12•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=69b3dae02d91 waiting for green on try
|
|
Assignee | |
Updated•12 years ago
|
Keywords: checkin-needed
|
||
Comment 13•12 years ago
|
||
Any reason to believe those failures aren't from this patch?
Keywords: checkin-needed
|
|
Assignee | |
Comment 14•12 years ago
|
||
I commented a few of them. Let me rebase the patch an try again, but I don't see any possible relationship between this patch and these bugs.
|
|
Assignee | |
Comment 15•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1c0492aad4c4
|
|
Assignee | |
Comment 16•12 years ago
|
||
Definitely it doesn't seem failures related to this patch. This try report is almost all green.
|
|
Assignee | |
Updated•12 years ago
|
Keywords: checkin-needed
|
||
Comment 17•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b6048b626a
Keywords: checkin-needed
|
||
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/79b6048b626a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•