crash [@mozStorageResultSet::GetNextRow(mozIStorageRow**) ]

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Toolkit
Storage
--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: sdwilsh, Assigned: ddahl)

Tracking

({crash})

1.9.1 Branch
mozilla1.9.3a1
crash
Points:
---
Bug Flags:
blocking1.9.2 -

Firefox Tracking Flags

(status1.9.2 beta3-fixed, status1.9.1 .6-fixed)

Details

(Whiteboard: [good first bug], crash signature, URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Only seeing one crash here, but this just needs a simple null check on the return value.  NS_ENSURE_ARG_POINTER anyone?
(Reporter)

Updated

8 years ago
Whiteboard: [good first bug]
(Reporter)

Updated

8 years ago
(Reporter)

Comment 1

8 years ago
I suppose it's also possible for GetNextRow to fail here by getting null out of the mData array.  Although, I don't see how that could be null.
(Reporter)

Comment 2

8 years ago
FWIW, checking the argument for non-nullness is what we should do.  If we continue to see crashes with this signature, we'll know it's the later (and be confused).
(Assignee)

Updated

8 years ago
Assignee: nobody → ddahl
(Assignee)

Comment 3

8 years ago
Are you looking for something like this, or is there a macro I should be using?

NS_IMETHODIMP
ResultSet::GetNextRow(mozIStorageRow **_row)
{
  if(!*_row){
    // Return an error?
    return NS_OK;
  }

  if (mCurrentIndex >= mData.Count()) {
    // Just return null here
    return NS_OK;
  }

  NS_ADDREF(*_row = mData.ObjectAt(mCurrentIndex++));
  return NS_OK;
}
(Reporter)

Comment 4

8 years ago
I hinted at the macro in comment 0 ;)
(Assignee)

Comment 5

8 years ago
(In reply to comment #4)
> I hinted at the macro in comment 0 ;)
So I tried:

NS_ENSURE_ARG_POINTER(*_row);

and I see this warning in the test suite:

WARNING: NS_ENSURE_TRUE(*_row) failed: file /home/ddahl/code/moz/mozilla-central/mozilla/storage/src/mozStorageResultSet.cpp, line 80


then a test failure:

TEST-UNEXPECTED-FAIL | /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_statement_executeAsync.js | 0 == 11 - See following stack:
JS frame :: /home/ddahl/code/moz/mozilla-central/mozilla/testing/xpcshell/head.js :: do_throw :: line 197
JS frame :: /home/ddahl/code/moz/mozilla-central/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 227
JS frame :: /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_statement_executeAsync.js :: anonymous :: line 996
(Assignee)

Comment 6

8 years ago
Created attachment 408731 [details] [diff] [review]
v 0.1 WIP - tests fail

the above mentioned tests fail with this patch:(
(Reporter)

Comment 7

8 years ago
(In reply to comment #5)
> NS_ENSURE_ARG_POINTER(*_row);
You do not want to dereference _row.  That is what we are trying to protect against crashing on.
(Assignee)

Comment 8

8 years ago
Created attachment 408882 [details] [diff] [review]
v 0.2 All tests pass

Is a test required as well, or is this code path tested well enough?
Attachment #408731 - Attachment is obsolete: true
Attachment #408882 - Flags: review?(sdwilsh)
(Reporter)

Comment 9

8 years ago
(In reply to comment #8)
> Is a test required as well, or is this code path tested well enough?
Can't really write a test for this unless we write a native code test.  It's just a null check, and I'm happy with this.
(Reporter)

Comment 10

8 years ago
Comment on attachment 408882 [details] [diff] [review]
v 0.2 All tests pass

r=sdwilsh

Would like to see this on branches.
Attachment #408882 - Flags: review?(sdwilsh)
Attachment #408882 - Flags: review+
Attachment #408882 - Flags: approval1.9.2?
Attachment #408882 - Flags: approval1.9.1.5?
(Reporter)

Updated

8 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4f972fa92cd0
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 408882 [details] [diff] [review]
v 0.2 All tests pass

Approved for 1.9.1.6, a=dveditz for release-drivers
Attachment #408882 - Flags: approval1.9.1.6? → approval1.9.1.6+
Flags: blocking1.9.2?
Comment on attachment 408882 [details] [diff] [review]
v 0.2 All tests pass

Approved for 1.9.2 - simple hygiene. Don't think we'd hold the release for this bug though, so I'll blocking- this momentarily.
Attachment #408882 - Flags: approval1.9.2? → approval1.9.2+
Flags: blocking1.9.2? → blocking1.9.2-
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9cdb04347c82
status1.9.2: --- → final-fixed
Keywords: checkin-needed
Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f9e848c13288
status1.9.1: --- → .6-fixed
Severity: normal → critical
Keywords: crash
Crash Signature: [@mozStorageResultSet::GetNextRow(mozIStorageRow**) ]
You need to log in before you can comment on or make changes to this bug.