Closed Bug 555087 Opened 10 years ago Closed 4 years ago

crash [@mozilla::storage::Statement::BindParameters(mozIStorageBindingParamsArray*)]

Categories

(Toolkit :: Storage, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ancestor.ak, Assigned: tranj23, Mentored)

Details

(Whiteboard: [good first bug][lang=cpp])

Attachments

(3 files, 1 obsolete file)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a4pre) Gecko/20100325 Minefield/3.7a4pre

The crash happens if you call mozIStorageStatement.bindParameters() from JS with an undefined argument.

Crash ID: ee64897a-6610-4090-b91c-8aa002100325
We need an NS_ENSURE_ARG there...
Flags: in-testsuite?
Whiteboard: [good first bug]
That crash id was from Firefox 3.6, latest trunk has a slightly different signature.

bp-75570c40-9cb3-416d-9c6c-6b8652100325
Summary: crash [@mozilla::storage::BindingParams::getOwner()] → crash [@mozilla::storage::Statement::BindParameters(mozIStorageBindingParamsArray*)]
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #437732 - Flags: review?(sdwilsh)
Comment on attachment 437732 [details] [diff] [review]
Patch

> NS_IMETHODIMP
> Statement::BindParameters(mozIStorageBindingParamsArray *aParameters)
> {
>   if (!mDBStatement)
>     return NS_ERROR_NOT_INITIALIZED;
> 
>+  NS_ENSURE_ARG_POINTER(aParameters);
Would prefer this to be on the first line.

r- only because this patch has no test.  Probably want to add it to test_statement_executeAsync.js.  Also, I suggest you ask for review from :asuth as I'm going to be out for the next week.
Attachment #437732 - Flags: review?(sdwilsh) → review-
Attached patch WIPSplinter Review
What else could be going on here? This patch indeed makes the browser throw a JS Exception instead of the horrifying segmentation fault. However, running the xpcshell tests still results in a process crash (I'm still getting "MINIDUMP_STACKWALK not set, can't process dump.")
Attachment #437732 - Attachment is obsolete: true
So I got the minidump stack walk sorted out, but all I'm getting is:

PROCESS-CRASH | /home/saint/mozilla/obj/_tests/xpcshell/test_storage/unit/test_statement_executeAsync.js | application crashed (minidump found)
Traceback (most recent call last):
  File "/home/saint/mozilla/mc/config/pythonpath.py", line 40, in <module>
    execfile(script, {'__name__': '__main__', '__file__': script})
  File "/home/saint/mozilla/mc/testing/xpcshell/runxpcshelltests.py", line 525, in <module>
    main()
  File "/home/saint/mozilla/mc/testing/xpcshell/runxpcshelltests.py", line 521, in main
    if not xpcsh.runTests(args[0], testdirs=args[1:], **options.__dict__):
  File "/home/saint/mozilla/mc/testing/xpcshell/runxpcshelltests.py", line 457, in runTests
    checkForCrashes(testdir, self.symbolsPath, testName=test)
  File "/home/saint/mozilla/mc/build/automationutils.py", line 112, in checkForCrashes
    subprocess.call([stackwalkPath, d, symbolsPath], stderr=nullfd)
  File "/usr/lib/python2.6/subprocess.py", line 470, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.6/subprocess.py", line 621, in __init__
    errread, errwrite)
  File "/usr/lib/python2.6/subprocess.py", line 1126, in _execute_child
    raise child_exception
OSError: [Errno 13] Permission denied
make: *** [check-one] Error 1
Just to clarify, I didn't expect permission errors while running as root but I still got this message, and none of it looks related to the object code (which is what I was expecting)...
You should be using expectError [1] here to check it.  Not clear why you are crashing, but it could be a test harness issue.  I generally use check-interactive [2] to get good stack traces.  Make sure you've rebuilt everything you need to.

[1] http://mxr.mozilla.org/mozilla-central/source/storage/test/unit/head_storage.js#163
[2] https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests#Running_unit_tests_under_a_C.2b.2b_debugger
Saint, are you still interested in working on this bug? Would you like any help? Thanks!
Flags: needinfo?(wesongathedeveloper)
Sorry for the lag here. Unassigning myself.
Status: ASSIGNED → NEW
Assignee: wesongathedeveloper → nobody
Flags: needinfo?(wesongathedeveloper)
Still crashes!
Mentor: adw
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [good first bug] → [good first bug][lang=cpp]
Hi,

I am new to open source development. Could I start working on this bug?
Flags: needinfo?(ancestor.ak)
Hi Nivedhita,

Sure, thanks for volunteering!  This should be a simple bug to fix.  But first you need to set up a Firefox build environment.  If you haven't done that yet, please see these links:

https://developer.mozilla.org/en-US/docs/Introduction
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

Once you're able to build and run Firefox, you can start working on this bug.  Actually Saint Wesonga already posted a patch, above, that just needs to be updated.  You can try applying it to your own tree and working on it, although it's pretty old and may not apply cleanly anymore, or you can start from scratch.

You need to add NS_ENSURE_ARG(aParameters) here: http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatement.cpp?rev=aa3aec12cc77#580

And then you need to write a test.  You can add a test to this file like Saint Wesonga's patch does: http://mxr.mozilla.org/mozilla-central/source/storage/test/unit/test_statement_executeAsync.js

Like Shawn says in comment 8, use the expectError function to make sure passing undefined throws an error.  This type of test is an xpcshell test, which you can read about here: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

Before you post your patch, run the test on your own computer to make sure it passes.

Let me know when you have questions.
Flags: needinfo?(ancestor.ak)
Thank you. I have built and run firefox once. I will look at Saint Wesonga's patch and start working on the patch myself.
Hi Nivedhita, have you been able to work on this?  I'm ready to help you with any questions you might have.
Could i start working on this bug as my first bug?
Sure, Vincent, please see comment 13 for how to get started.  Let me know here in the bug when you have questions.
Comment on attachment 8632466 [details] [diff] [review]
Patch for bug 555087 - Does not longer crash, but throws an exception if mozIStorageStatement::bindParameters() is called with undefined

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

Thanks for the patch.  Looks good, just a couple of things.  Please fix them and post a new patch.

::: storage/test/unit/test_storage_statement.js
@@ +172,5 @@
> +  try {
> +    stmt.bindParameters(undefined);
> +  } catch (e) {
> +    //A exception is expected. The thing we want to prevent is a crash.
> +  }

Please also check that the exception is actually thrown.  I said earlier that you should use an expectError function, but that's outdated, sorry.  Instead, please use Assert.throws().  You can read about it here: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging

Use /NS_ERROR_ILLEGAL_VALUE/ as the regular expression that the error message should match.

@@ +184,5 @@
>               test_getColumnName, test_getColumnIndex_same_case,
>               test_getColumnIndex_different_case, test_state_ready,
>               test_state_executing, test_state_after_finalize,
>               test_getColumnDecltype,
>               test_failed_execute,

You need to add the name of your new function to the end of this array.  Otherwise it won't be called.
Attachment #8632466 - Flags: review?(adw) → feedback+
(In reply to Drew Willcoxon :adw from comment #19)
> Please also check that the exception is actually thrown.  I said earlier
> that you should use an expectError function, but that's outdated, sorry. 

Er, I was right the first time.  Do use expectError, whose definition is linked in comment 8.  Pass Cr.NS_ERROR_ILLEGAL_VALUE as the aErrorCode argument.
I created this patch based on previous patches and suggestions in the comments. Then I ran this patch on my computer and it passed.
Attachment #8710195 - Flags: review?(adw)
Comment on attachment 8710195 [details] [diff] [review]
rev1 - Added statement to mozStorageStatement.cpp and added a test to test_storage_statement.js

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

Thanks Jeffrey!
Attachment #8710195 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/90bd68144f35
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.