1007034 - Show a console warning when using deprecated mozIStorageBaseStatement methods

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect, P2)

Description

[Marco Bonardo [:mak]]

we should show a console warning when someone uses the deprecated methods in mozIStorageBaseStatement, to reduce their usage and finally be able to remove them.

Comment 1

[Mike Hoye [:mhoye]]

Moving this to good-next, this looks straightforward but isn't a starter bug.

Comment 2

[Shashank VRSN Sabniveesu]

How to identify methods as 'deprecated'? What files are we looking at?

Comment 3

[Marco Bonardo [:mak]]

Sorry, comment 0 was pretty bad and lacking.
Most of the information is in bug 645049, the deprecated methods are in mozIStorageBaseStatement and more specifically:

bindUTF8StringParameter		
bindStringParameter		
bindDoubleParameter		
bindInt32Parameter		
bindInt64Parameter		
bindNullParameter
bindBlobParameter

to warn about deprecation in the console one may do something like this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsPlacesMacros.h#47
and then add the deprecation macro in the methods

Comment 4

[Timur Valeev]

Is it still actual?

Comment 5

[Marco Bonardo [:mak]]

yes it is, we also need to fix the only in-tree consumer of these APIs
http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#1280
using BindXXXByName() APIs.

Comment 6

[Devansh D]

Hello guys, I want to fix this one.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Devansh D from comment #6)
> Hello guys, I want to fix this one.

Freel free to attach a patch or ask questions as you need.

Comment 8

[sajid.ahmed]

Hi Marco,
I want to fix this. But before I begin, I want to know what is a good way to test this. Looking for some insight.

Comment 9

[Marco Bonardo [:mak]]

I don't think it's worth to write an automated test for this, we usually don't. But yes, you need a way to test this locally.

For your local testing you could use your debug build:
1. Open devtools and enable Browser chrome and add-on debugging toolboxes
2. Open Scratchpad (from devtools) and select the Browser environment (this is important)
3. In Scratchpad you can write your code, and save it locally so you don't have to rewrite it every time
4. In your code open a connection to a memory database (openSpecialDatabase)
5. Create a temp table with 7 columns (executeSimpleSQL)
6. write a statement that binds 7 parameters(createStatement) to those 7 columns
7. use the deprecated APIs to bind the 7 parameters
8. select the whole code and run it, check that you get the 7 deprecated warnings in the Browser Console

Re: consumers, this is an updated search for them:
http://searchfox.org/mozilla-central/search?q=Bind%5Ba-z%5D%2BParameter%5C%28&regexp=true&path= (ignore anything in __GENERATED__)
It's basically only dom/indexedDB/ActorsParent.cpp that has its own tests and we'll run them on the Try server once we have a patch.

An updated link to a possible deprecation macro (should be copied and then modified into storage/mozStorageHelper.h)
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsPlacesMacros.h#68

Feel free to ask questions.

Comment 10

[sajid.ahmed]

Thanks for your detailed response, I was able to setup the test cases.

However, I am unable to find the implementations of the above methods which have been deprecated.
I located them in the storage/mozIStorageBaseStatement.idl file, but as it turns out they are only declarations. I am new to idl files, and any help with regards to finding the implementations of the above methods would be appreciated.

P.S. I ran `grep` with the deprecated method names in the entire tree, and the only other usages I found where in some of the build directories.

Comment 11

[Marco Bonardo [:mak]]

Ah, there's a lot of fancy stuff here indeed.
BOILERPLATE_BIND_PROXIES generates the implementations through BIND_GEN_IMPL and forwards the calls to BindbyName and BindByIndex.

In particular, the BindXXXParameter methods are implemented here (notice BIND_NAME_CONCAT(_name, Parameter)):

http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/storage/StorageBaseStatementInternal.h#237
We can add our notification just before _guard.
We should also add an if (NS_IsMainThread()) check before instantiating the console service, and MOZ_ASSERT instead of NS_WARNING.

Comment 12

[sajid.ahmed]

Attachment: Show console warnings when using (deprecated) BindXXXParameter methods in mozIStorageBaseStatement.

I have wrote the following tests for verifying the changes in the attached patch. I am adding it below in case it helps in saving some valuable finger muscles.

// ==============================================================

Components.utils.import("resource://gre/modules/Services.jsm");
Components.utils.import("resource://gre/modules/FileUtils.jsm");


dropTable = "DROP TABLE IF EXISTS test_data;"

createTable = "CREATE TABLE test_data (\
  utf8_string_col TEXT,\
  string_col TEXT,\
  double_col REAL,\
  int32_col INTEGER,\
  int64_col INTEGER,\
  null_col INTEGER,\
  blob_col BLOB\
);";

insertDummyData = "INSERT INTO test_data VALUES(?1, ?2, ?3, ?4, ?5, ?6, ?7);";


// Open connection to the test database
file = FileUtils.getFile("ProfD", ["test_db.sqlite"]);
dbConn = Services.storage.openDatabase(file);

// Create a table for inserting different types (i.e. string, int, etc.) of dummy data
dbConn.executeSimpleSQL(dropTable);
dbConn.executeSimpleSQL(createTable);

// Calling the deprecated bind methods will now generate a warning message.
statement = dbConn.createStatement(insertDummyData);
statement.bindUTF8StringParameter(0, "utf8-string");
statement.bindStringParameter(1, "string")
statement.bindDoubleParameter(2, 89.6);
statement.bindInt32Parameter(3, 101);
statement.bindInt64Parameter(4, Number.MAX_SAFE_INTEGER);
statement.bindNullParameter(5);
blob = [1, 2, 3, 4, 5, 6];
statement.bindBlobParameter(6, blob, blob.length);

// Close connection to the database
dbConn.close();
file.remove(false);

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8840176 [details] [diff] [review]
Show console warnings when using (deprecated) BindXXXParameter methods in mozIStorageBaseStatement.

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

Could you please also fix the 3 deprecated calls here :
http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/dom/indexedDB/ActorsParent.cpp#1299
It should be enough to replace Parameter with ByIndex, afaict.

::: storage/mozStorageHelper.h
@@ +212,5 @@
> +#define WARN_DEPRECATED()                                                        \
> +  PR_BEGIN_MACRO                                                                 \
> +  nsCString msg(__FUNCTION__);                                                   \
> +  msg.AppendLiteral(" is deprecated and will be removed soon.");                 \
> +  MOZ_ASSERT(msg.get());                                                         \

this should be MOZ_ASSERT(false, msg.get()); or it won't ever happen

Comment 14

[Marco Bonardo [:mak]]

note that in a debug build MOZ_ASSERT will crash.

Comment 15

[sajid.ahmed]

Attachment: 1) Added console warnings for the deprecated BindXXXParameter methods in mozIStorageBaseStatement. 2) Replaced calls to the above deprecated methods in dom/indexedDB/ActorsParent.cpp.

Comment 16

[sajid.ahmed]

Thanks for pointing these out - I have attached a new patch containing all changes so far. Let me know if there is anything else that should be included with this.

Also, I wanted to mention that I am having some trouble setting up the project in Eclipse. Although, it compiles and runs fine, it keeps giving me all sorts of compiler errors like "method could not be resolved", "the type <a> must implement the pure virtual method <b>", etc. There is no specific reason as to why I chose Eclipse (over others) - I just need an IDE which will allow me to jump to method declarations, find usages, show syntax errors / warnings, and have auto-completion. It'll be nice if you can provide some help / insight about this, or what you have been using for your dev work.

Comment 17

[Marco Bonardo [:mak]]

I never tried Eclipse, but I know someone in Mozilla does.
Did you  try to have a look at this document? https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Eclipse/Eclipse_CDT
You could also try to ask about specific questions in the #developers channel or on the dev-platform mozilla mailing list.

Personally I am just using Visual Studio Code at the moment, I'm not so deep into IDEs.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8840607 [details] [diff] [review]
1) Added console warnings for the deprecated BindXXXParameter methods in mozIStorageBaseStatement.  2) Replaced calls to the above deprecated methods in dom/indexedDB/ActorsParent.cpp.

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

It looks good, I will push it to the Try Server to verify tests

Comment 19

[Marco Bonardo [:mak]]

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

Comment 20

[sajid.ahmed]

Thanks. That document looks helpful. I'll give it a try soon.

Btw, I am not fully sure how to interpret the Try server test results. There seems to be a few failures, but not sure if they are expected.

Comment 21

[Marco Bonardo [:mak]]

hm, no the red failures were not expected, they only happen in debug builds, looks like the macro is not expanded properly "missing ')' before identifier 'msg'"
I suspect MOZ_ASSERT wants a string literal and not a pointer. In such a case we may have to use a simpler message in the MOZ_ASSERT and move the msg string inside the if.
Did you build optimized or debug?

Comment 22

[sajid.ahmed]

Attachment: Added console warnings for the deprecated BindXXXParameter methods in mozIStorageBaseStatement, and replaced their usages in dom/indexedDB/ActorsParent.cpp

I initially did not verify using the debug build - my bad. I have verified with both the debug and the regular builds this time. It prints the expected message to the console, and on debug builds MOZ_ASSERT causes the app to crash (as expected).

I have passed a string literal to MOZ_ASSERT in order to get around the issue we were having.

Hopefully, this patch should do.

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 8854635 [details] [diff] [review]
Added console warnings for the deprecated BindXXXParameter methods in mozIStorageBaseStatement, and replaced their usages in dom/indexedDB/ActorsParent.cpp

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

Pushed to Try to check tests.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=411ddd62e0c71ee0da88bf54dcb8e74b9cd47b68

::: storage/mozStorageHelper.h
@@ +226,5 @@
> +        cs->LogMessage(e);                                                       \
> +      }                                                                          \
> +    }                                                                            \
> +  }                                                                              \
> +  MOZ_ASSERT(false, "You are trying to use a deprecated method. "                \

I'd probably add mozStorage there, so "a deprecated mozStorage method."

@@ +227,5 @@
> +      }                                                                          \
> +    }                                                                            \
> +  }                                                                              \
> +  MOZ_ASSERT(false, "You are trying to use a deprecated method. "                \
> +                    "Check console warning to identify the method name.");       \

nit: it's actually an error, not a warning.

Comment 24

[Marco Bonardo [:mak]]

Try looks ok, the failures are unrelated, even if it was an unlucky push!
We can land, once you attach the final patch.

Comment 25

[sajid.ahmed]

Attachment: DeprecationWarning.patch

I made the changes you suggested.

Also, is there a way I would know when my patch gets landed?

Comment 26

[Marco Bonardo [:mak]]

Everything is annotated in the bug, so you will notice when the bug lands. It will first land in an integration tree, and then it will be marked fixed once the integration branch merges into mozilla-central.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2f7bd2c69db
Add console warnings for the deprecated BindXXXParameter methods in mozIStorageBaseStatement and replace calls to the deprecated methods in dom/indexedDB/ActorsParent.cpp. r=mak

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2f7bd2c69db

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I think that crashing at running debug build is overreaction because add-ons may use it. So, this blocks me to test patched build with my default profile... (Perhaps, it shouldn't crash at called by JS.)

Comment 30

[Marco Bonardo [:mak]]

(In reply to Masayuki Nakano [:masayuki] from comment #29)
> Hmm, I think that crashing at running debug build is overreaction because
> add-ons may use it. So, this blocks me to test patched build with my default
> profile... (Perhaps, it shouldn't crash at called by JS.)

It's the only way that worked well so far to make developers aware of the deprecations. Nobody checks idl comments, people checking console spew end up not reporting the issue to add-on authors cause it doesn't pertain to them.
Plus when we'll remove the APIs the add-ons will just cause more issues, since they will stop working and maybe ever break things. 

I'm sorry about the possible disruptions, but please notify the add-on authors of the problem and temporarily disable them.

Comment 31

[Marco Bonardo [:mak]]

and as a matter of facts, these APIs were deprecated from YEARS, and indeed nobody cared.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It's difficult to check which addon causes the crash because of crashing at start up :-(

And I don't think that add-on developers check their add-ons with debug build, so, I still don't think crashing at running debug build makes sense...

Comment 33

[Marco Bonardo [:mak]]

it actually helped finding call points in Thunderbird and will prevent our own code from using these APIs, since debug tests would crash.

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

Maybe it would help if we invoked nsIXPConnect::debugDumpJSStack (http://searchfox.org/mozilla-central/source/js/xpconnect/idl/nsIXPConnect.idl#426) prior to asserting if on the main thread?  The native stack is going to be useless for JS callers like addons and that seems like an easy way to log the JS stack from C++.

Comment 35

[Marco Bonardo [:mak]]

I'd be fine with doing that, if it helps. I'll file a bug to add it to the deprecation warning.

Comment 36

[Daniel Holbert [:dholbert]]

The assertion message could use a bit of wordsmithing here, I think -- right now, it says:
====
Assertion failure: false (You are trying to use a deprecated mozStorage method. Check error message in console to identify the method name.)
====

But the suggested action-item -- "check error message in console" -- isn't actually actionable, because (a) Firefox's error console isn't available because we aborted already, and (b) the error that was logged to Firefox's error console does not appear anywhere else AFAICT (it's not in my terminal before the abort).

Comment 37

[Daniel Holbert [:dholbert]]

sajid, perhaps you could post a followup patch to adjust the message here?

Probably just drop the "Check error message in console" part (since as noted above, that's impossible as far as I can see).  Once bug 1355084 lands, it looks like we'll be outputting that text to the terminal anyway, so there won't be any need to direct people towards the (inaccessible) console.

Comment 38

[Daniel Holbert [:dholbert]]

(comment 37 is assuming mak agrees with me, of course.)

Comment 39

[Marco Bonardo [:mak]]

we are going for bug 1355084 (that I was actually fixing in bug 1355561, but got stolen by other higher priorities)
I'd suggest to post suggestions to that bug since this one is resolved.
Yes, once we dump the js stack we can probably stop telling about the console.