Last Comment Bug 524787 - crash [@mozStorageResultSet::GetNextRow(mozIStorageRow**) ]
: crash [@mozStorageResultSet::GetNextRow(mozIStorageRow**) ]
[good first bug]
: crash
Product: Toolkit
Classification: Components
Component: Storage (show other bugs)
: 1.9.1 Branch
: All All
: -- critical (vote)
: mozilla1.9.3a1
Assigned To: David Dahl :ddahl
: Marco Bonardo [::mak]
Depends on:
  Show dependency treegraph
Reported: 2009-10-27 14:20 PDT by Shawn Wilsher :sdwilsh
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
bugzilla: blocking1.9.2-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v 0.1 WIP - tests fail (609 bytes, patch)
2009-10-27 16:32 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.2 All tests pass (608 bytes, patch)
2009-10-28 11:13 PDT, David Dahl :ddahl
sdwilsh: review+
bugzilla: approval1.9.2+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review

Description Shawn Wilsher :sdwilsh 2009-10-27 14:20:09 PDT
Only seeing one crash here, but this just needs a simple null check on the return value.  NS_ENSURE_ARG_POINTER anyone?
Comment 1 Shawn Wilsher :sdwilsh 2009-10-27 14:34:20 PDT
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.
Comment 2 Shawn Wilsher :sdwilsh 2009-10-27 14:35:31 PDT
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).
Comment 3 David Dahl :ddahl 2009-10-27 15:14:39 PDT
Are you looking for something like this, or is there a macro I should be using?

ResultSet::GetNextRow(mozIStorageRow **_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;
Comment 4 Shawn Wilsher :sdwilsh 2009-10-27 15:47:54 PDT
I hinted at the macro in comment 0 ;)
Comment 5 David Dahl :ddahl 2009-10-27 16:06:42 PDT
(In reply to comment #4)
> I hinted at the macro in comment 0 ;)
So I tried:


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
Comment 6 David Dahl :ddahl 2009-10-27 16:32:40 PDT
Created attachment 408731 [details] [diff] [review]
v 0.1 WIP - tests fail

the above mentioned tests fail with this patch:(
Comment 7 Shawn Wilsher :sdwilsh 2009-10-27 16:32:54 PDT
(In reply to comment #5)
You do not want to dereference _row.  That is what we are trying to protect against crashing on.
Comment 8 David Dahl :ddahl 2009-10-28 11:13:29 PDT
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?
Comment 9 Shawn Wilsher :sdwilsh 2009-10-30 12:45:36 PDT
(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.
Comment 10 Shawn Wilsher :sdwilsh 2009-10-30 12:46:20 PDT
Comment on attachment 408882 [details] [diff] [review]
v 0.2 All tests pass


Would like to see this on branches.
Comment 11 Dão Gottwald [:dao] 2009-10-31 03:56:53 PDT
Comment 12 Daniel Veditz [:dveditz] 2009-11-04 16:54:22 PST
Comment on attachment 408882 [details] [diff] [review]
v 0.2 All tests pass

Approved for, a=dveditz for release-drivers
Comment 13 Johnathan Nightingale [:johnath] 2009-11-10 11:33:32 PST
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.
Comment 15 Justin Dolske [:Dolske] 2009-11-11 00:19:37 PST
Pushed to 191:

Note You need to log in before you can comment on or make changes to this bug.