Closed Bug 557346 Opened 14 years ago Closed 14 years ago

The .message of a DOM Worker error event is not populated when the worker does |throw new Error("data");|

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Version:
Other Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- .4-fixed
status1.9.1 --- .10-fixed

People

(Reporter: jorendorff, Assigned: mrbkap)

References

Details

Attachments

(1 file)

Proposed fix
1.03 KB, patch
jorendorff
: review+
beltzner
: approval1.9.2.4+
beltzner
: approval1.9.1.10+
Details | Diff | Splinter Review 
<!DOCTYPE html>
<html>
<head>
<script>
    var w = Worker("fail.js");  // throw new Error("fail") on first message.
    w.onerror = function (event) {
        alert("event.message is:" + uneval(event.message) + "\n" +
        "event.filename is:" + event.filename + "\n" +
        "event.lineno is:" + event.lineno);
    };
    w.postMessage("hello");
</script>
</head>
<body>
</body>
</html>

Expected: The worker's onerror handler should receive an event with
event.message === "data".

Observed: event.message === "".

Filing in JavaScript Engine for now, because bent took a look and says the JSErrorReport's ucmessage field is NULL. I'm not sure what that field is supposed to mean; perhaps not what http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMThreadService.cpp#553 thinks it means. If so, it's still partly our bug since this isn't documented anywhere and I should at least be able to tell bent what the right thing would be.
To clarify, the |aMessage| param is correct in this case, but the |ucMessage| member of the error report is null. Should I always be using |aMessage| whenever possible?
Attached patch Proposed fix — — Splinter Review 
IMO this is just a bug -- we should fill in report->ucmessage whenever we can (especially to avoid chopping wide characters passed to the Error constructor). I still think that the worker code should deal with not having a ucmessage, and fall back onto using |message| in that case.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #437939 - Flags: review?
Attachment #437939 - Flags: review? → review?(jorendorff)
Attachment #437939 - Flags: review?(jorendorff) → review+
Comment on attachment 437939 [details] [diff] [review]
Proposed fix

Thanks.
http://hg.mozilla.org/mozilla-central/rev/34eb552d42f5
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 437939 [details] [diff] [review]
Proposed fix

I'd really like to get this on 1.9.2. It fixes a hole in the error reporting mechanism that has been around for a while and breaks simple uses of 'throw new Error()' in script.
Attachment #437939 - Flags: approval1.9.2.4?
Comment on attachment 437939 [details] [diff] [review]
Proposed fix

a=beltzner for 1.9.2.4
Attachment #437939 - Flags: approval1.9.2.4? → approval1.9.2.4+
Comment on attachment 437939 [details] [diff] [review]
Proposed fix

Do we want this on 1.9.1? Simple patch, no real risk, fixes the error report mechanism when doing a |throw new Error();| which is probably pretty common...
Attachment #437939 - Flags: approval1.9.1.10?
Comment on attachment 437939 [details] [diff] [review]
Proposed fix

a=beltzner for 1.9.1.10
Attachment #437939 - Flags: approval1.9.1.10? → approval1.9.1.10+
Friendly notice: both the 1.9.1.10 and 1.9.2.4 code freezes are scheduled for *tomorrow*, Tuesday April 27th 2010 @ 11:59 pm PST.
Assignee: mrbkap → bent.mozilla
Blake's patch doesn't apply cleanly to 1.9.1, he's going to take a peek and make sure it's a safe merge.
Assignee: bent.mozilla → mrbkap
Ah, thanks Ben. Sorry for assigning this to you prematurely.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: