Closed Bug 1168593 Opened 6 years ago Closed 6 years ago

AutoEntryScript::DocshellEntryMonitor::Entry does not check if initTwoByte fails

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: mccr8, Assigned: tromey)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1299624])

Attachments

(1 file, 1 obsolete file)

All 37 other places initTwoByte is called, the return value is checked. Maybe it should be checked here, too?  It might also be worth adding MOZ_WARN_UNUSED_RESULT
 to initTwoByte.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Here's the patch.  I also made init use MOZ_WARN_UNUSED_RESULT to be
consistent.  This caused no additional issues.

I don't know a good way to write a test for this.
Attachment #8610843 - Flags: review?(jdemooij)
(In reply to Tom Tromey :tromey from comment #1)
> I don't know a good way to write a test for this.

There's no need for a test. With the annotation, then in any directories that are warnings-as-errors, we'll just get compile time failure.
(In reply to Andrew McCreight [:mccr8] from comment #2)
> (In reply to Tom Tromey :tromey from comment #1)
> > I don't know a good way to write a test for this.
> 
> There's no need for a test. With the annotation, then in any directories
> that are warnings-as-errors, we'll just get compile time failure.

I was unclear - I meant a test for the logic change in ScriptSettings.
There's no way to test the failure path.  Of course this was a problem in
the previous code as well, it was just more obscure.
Comment on attachment 8610843 [details] [diff] [review]
make AutoStableStringChars::init use MOZ_WARN_UNUSED_RESULT

Review of attachment 8610843 [details] [diff] [review]:
-----------------------------------------------------------------

Good find.
Attachment #8610843 - Flags: review?(jdemooij) → review+
Comment on attachment 8610843 [details] [diff] [review]
make AutoStableStringChars::init use MOZ_WARN_UNUSED_RESULT

Review of attachment 8610843 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, after taking a closer look at the code, I'm wondering if this was intentional (if it is, there should have been a comment though..)

Below it checks functionName.isTwoByte(), maybe that's supposed to be false if init() failed? Can you track down who added this code and discuss with him/her?
Attachment #8610843 - Flags: review+
(In reply to Jan de Mooij [:jandem] from comment #5)
> Comment on attachment 8610843 [details] [diff] [review]
> make AutoStableStringChars::init use MOZ_WARN_UNUSED_RESULT
> 
> Review of attachment 8610843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, after taking a closer look at the code, I'm wondering if this was
> intentional (if it is, there should have been a comment though..)
> 
> Below it checks functionName.isTwoByte(), maybe that's supposed to be false
> if init() failed? Can you track down who added this code and discuss with
> him/her?

I wrote it.  I didn't consider the initTwoByte return value and then
it wasn't caught in review.  So the idea of this patch is to make it
so that it will cause a build failure instead.

I think it's fine for this code to have an early return on this kind of
failure.  It's consistent with how errors are generally treated in the timeline.
Comment on attachment 8610843 [details] [diff] [review]
make AutoStableStringChars::init use MOZ_WARN_UNUSED_RESULT

Ah, right.

(This may also leave a pending OOM exception on the JSContext, but that's a bigger problem inside and outside the engine and orthogonal to this fix.)
Attachment #8610843 - Flags: review+
(In reply to Jan de Mooij [:jandem] from comment #7)
> Comment on attachment 8610843 [details] [diff] [review]
> make AutoStableStringChars::init use MOZ_WARN_UNUSED_RESULT
> 
> Ah, right.
> 
> (This may also leave a pending OOM exception on the JSContext, but that's a
> bigger problem inside and outside the engine and orthogonal to this fix.)

Oh, for some reason I was under the impression that this did not happen.
But of course it does.
I don't mind fixing it here - IIUC I just need to call JS_ClearPendingException
in the failure case.
Added the call to JS_ClearPendingException.
Attachment #8610843 - Attachment is obsolete: true
Attachment #8612460 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4037d0c890fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.