Closed Bug 387478 Opened 13 years ago Closed 13 years ago

NS_ScriptErrorReporter() doesn't need to null-check the report.

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Assigned: sciguyryan)

Details

Attachments

(1 file, 3 obsolete files)

Brendan says we don't need to null-check the JSErrorReport in NS_ScriptErrorReporter() any more, we're guaranteed to get a non-null report.
Attached patch Patch v1.0 (obsolete) — Splinter Review
Patch v1.0

Like so?
Assignee: nobody → sciguyryan
Status: NEW → ASSIGNED
Attachment #271642 - Flags: superreview?(jst)
Attachment #271642 - Flags: review?(jst)
Attached patch Patch v1 (-w) (obsolete) — Splinter Review
Patch v1 (-w)
Comment on attachment 271642 [details] [diff] [review]
Patch v1.0

Yes, only this happens to be against a tree w/o my crash fix that was needed after I landed my latest change to NS_ScriptErrorReporter(). You'll need to find that change (i.e. sameOrigin = report->filename == nsnull) and stick that in here and this'll look great! Update that and I'll r+sr. Thanks for the patch!
Attachment #271642 - Flags: superreview?(jst)
Attachment #271642 - Flags: superreview-
Attachment #271642 - Flags: review?(jst)
Attachment #271642 - Flags: review-
Attached patch Patch v1.0 (obsolete) — Splinter Review
Patch v1.1

Updated to include the latest tree changes (my build was a few days out-of-date due to internet issues on this end).
Attachment #271642 - Attachment is obsolete: true
Attachment #271643 - Attachment is obsolete: true
Attachment #271693 - Flags: superreview?(jst)
Attachment #271693 - Flags: review?(jst)
Comment on attachment 271693 [details] [diff] [review]
Patch v1.0

-          if (report) {
+          if (message) {
+            rv = errorObject->Init(msg.get(), nsnull, nsnull, 0, 0, 0,
+                                   category);
+          }
+          else {
             PRUint32 column = report->uctokenptr - report->uclinebuf;
 
             rv = errorObject->Init(msg.get(), fileName.get(),
                                    reinterpret_cast<const PRUnichar*>
                                                    (report->uclinebuf),
                                    report->lineno, column, report->flags,
                                    category);

This isn't correct, we'll often (always?) get a non-null message as well as a report, so I think you simply want to use the report here, and never create an error object based on the message alone.

r+sr=jst with that.
Attachment #271693 - Flags: superreview?(jst)
Attachment #271693 - Flags: superreview+
Attachment #271693 - Flags: review?(jst)
Attachment #271693 - Flags: review+
OS: Linux → All
Hardware: PC → All
Version: unspecified → Trunk
Patch v1.2

Patch updated and ready for checkin.
Attachment #271693 - Attachment is obsolete: true
Keywords: checkin-needed
Fix landed on the trunk. Thanks for the patch!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: in-testsuite-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.