Closed
Bug 1168593
Opened 7 years ago
Closed 7 years ago
AutoEntryScript::DocshellEntryMonitor::Entry does not check if initTwoByte fails
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
1.99 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8610843 -
Flags: review?(jdemooij)
Reporter | ||
Comment 2•7 years ago
|
||
(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.
Assignee | ||
Comment 3•7 years ago
|
||
(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•7 years ago
|
||
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 5•7 years ago
|
||
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+
Assignee | ||
Comment 6•7 years ago
|
||
(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•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d88b87337b93
Assignee | ||
Comment 10•7 years ago
|
||
Added the call to JS_ClearPendingException.
Attachment #8610843 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8612460 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4037d0c890fa
Keywords: checkin-needed
Comment 12•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4037d0c890fa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•4 years ago
|
Blocks: coverity-analysis
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•