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

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Tom Tromey :tromey]

Attachment: make AutoStableStringChars::init use MOZ_WARN_UNUSED_RESULT

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.

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Tom Tromey :tromey]

(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 4

[Jan de Mooij [:jandem]]

Comment on attachment 8610843 [details] [diff] [review]
make AutoStableStringChars::init use MOZ_WARN_UNUSED_RESULT

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

Good find.

Comment 5

[Jan de Mooij [:jandem]]

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?

Comment 6

[Tom Tromey :tromey]

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

[Jan de Mooij [:jandem]]

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

Comment 8

[Tom Tromey :tromey]

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

Comment 9

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d88b87337b93

Comment 10

[Tom Tromey :tromey]

Attachment: make AutoStableStringChars::init use MOZ_WARN_UNUSED_RESULT

Added the call to JS_ClearPendingException.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4037d0c890fa

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4037d0c890fa