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

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, critical)

Description

[Adam Kowalczyk]

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

Comment 1

[Shawn Wilsher :sdwilsh]

We need an NS_ENSURE_ARG there...

Comment 2

[Adam Kowalczyk]

That crash id was from Firefox 3.6, latest trunk has a slightly different signature.

bp-75570c40-9cb3-416d-9c6c-6b8652100325

Comment 3

[Saint Wesonga]

Attachment: Patch

Comment 4

[Shawn Wilsher :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.

Comment 5

[Saint Wesonga]

Attachment: WIP

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

Comment 6

[Saint Wesonga]

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

Comment 7

[Saint Wesonga]

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

Comment 8

[Shawn Wilsher :sdwilsh]

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

Comment 9

[Liz Henry (:lizzard)]

Saint, are you still interested in working on this bug? Would you like any help? Thanks!

Comment 10

[Saint Wesonga]

Sorry for the lag here. Unassigning myself.

Comment 11

[Drew Willcoxon :adw (Away 12/23–1/1)]

Still crashes!

Comment 12

[Nivedhita S]

Hi,

I am new to open source development. Could I start working on this bug?

Comment 13

[Drew Willcoxon :adw (Away 12/23–1/1)]

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.

Comment 14

[Nivedhita S]

Thank you. I have built and run firefox once. I will look at Saint Wesonga's patch and start working on the patch myself.

Comment 15

[Drew Willcoxon :adw (Away 12/23–1/1)]

Hi Nivedhita, have you been able to work on this?  I'm ready to help you with any questions you might have.

Comment 16

[Vincent Woltman]

Could i start working on this bug as my first bug?

Comment 17

[Drew Willcoxon :adw (Away 12/23–1/1)]

Sure, Vincent, please see comment 13 for how to get started.  Let me know here in the bug when you have questions.

Comment 18

[Vincent Woltman]

Attachment: Patch for bug 555087 - Does not longer crash, but throws an exception if mozIStorageStatement::bindParameters() is called with undefined

Comment 19

[Drew Willcoxon :adw (Away 12/23–1/1)]

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.

Comment 20

[Drew Willcoxon :adw (Away 12/23–1/1)]

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

Comment 21

[Jeffrey Tran]

Attachment: rev1 - Added statement to mozStorageStatement.cpp and added a test to test_storage_statement.js

I created this patch based on previous patches and suggestions in the comments. Then I ran this patch on my computer and it passed.

Comment 22

[Drew Willcoxon :adw (Away 12/23–1/1)]

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!

Comment 23

[Drew Willcoxon :adw (Away 12/23–1/1)]

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

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/90bd68144f35

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90bd68144f35