Show a console warning when using deprecated mozIStorageBaseStatement methods

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Storage
P2
normal
RESOLVED FIXED
3 years ago
4 days ago

People

(Reporter: mak, Assigned: sajid.ahmed, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {addon-compat})

Trunk
mozilla55
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Keywords: addon-compat

Comment 1

3 years ago
Moving this to good-next, this looks straightforward but isn't a starter bug.
Whiteboard: [good first bug][mentor=mak][lang=cpp] → [good next bug][mentor=mak][lang=cpp]
How to identify methods as 'deprecated'? What files are we looking at?
Flags: needinfo?(mak77)
(Reporter)

Comment 3

3 years ago
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
Flags: needinfo?(mak77)
Mentor: mak77@bonardo.net
Whiteboard: [good next bug][mentor=mak][lang=cpp] → [good next bug][lang=cpp]
Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Updated

6 months ago
Priority: -- → P2
Whiteboard: [good next bug][lang=cpp] → [good first bug][lang=cpp]
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Comment 8

2 months ago
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.
(Reporter)

Comment 9

2 months ago
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.
(Assignee)

Comment 10

2 months ago
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.
(Reporter)

Comment 11

2 months ago
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.
(Assignee)

Comment 12

2 months ago
Created attachment 8840176 [details] [diff] [review]
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);
Attachment #8840176 - Flags: review?(mak77)
(Reporter)

Comment 13

2 months ago
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
Attachment #8840176 - Flags: review?(mak77) → feedback+
(Reporter)

Comment 14

2 months ago
note that in a debug build MOZ_ASSERT will crash.
(Assignee)

Comment 15

2 months ago
Created 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.
Attachment #8840176 - Attachment is obsolete: true
Attachment #8840607 - Flags: review?(mak77)
(Assignee)

Comment 16

2 months ago
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.
(Reporter)

Comment 17

2 months ago
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.
(Reporter)

Comment 18

2 months ago
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
Attachment #8840607 - Flags: review?(mak77) → review+
(Reporter)

Comment 19

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cf904736787b68fe64928a8419ad3619aa282a3
Flags: needinfo?(mak77)
(Assignee)

Comment 20

2 months ago
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.
(Reporter)

Comment 21

2 months ago
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?
Flags: needinfo?(mak77)
(Assignee)

Comment 22

21 days ago
Created attachment 8854635 [details] [diff] [review]
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.
Attachment #8840607 - Attachment is obsolete: true
Attachment #8854635 - Flags: review?(mak77)
(Reporter)

Comment 23

21 days ago
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.
Attachment #8854635 - Flags: review?(mak77) → review+
(Reporter)

Comment 24

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

Updated

21 days ago
Assignee: nobody → sajid.ahmed
Status: NEW → ASSIGNED
(Assignee)

Comment 25

21 days ago
Created attachment 8854983 [details] [diff] [review]
DeprecationWarning.patch

I made the changes you suggested.

Also, is there a way I would know when my patch gets landed?
Attachment #8854635 - Attachment is obsolete: true
Attachment #8854983 - Flags: review?(mak77)
(Reporter)

Updated

21 days ago
Attachment #8854983 - Flags: review?(mak77) → review+
(Reporter)

Comment 26

21 days ago
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.
Keywords: checkin-needed

Comment 27

20 days ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 28

20 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2f7bd2c69db
Status: ASSIGNED → RESOLVED
Last Resolved: 20 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

20 days ago
Depends on: 1354121
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.)
(Reporter)

Comment 30

16 days ago
(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.
(Reporter)

Comment 31

16 days ago
and as a matter of facts, these APIs were deprecated from YEARS, and indeed nobody cared.
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...
(Reporter)

Comment 33

16 days ago
it actually helped finding call points in Thunderbird and will prevent our own code from using these APIs, since debug tests would crash.
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++.
(Reporter)

Comment 35

16 days ago
I'd be fine with doing that, if it helps. I'll file a bug to add it to the deprecation warning.
(Reporter)

Updated

16 days ago
Blocks: 1355084
Depends on: 1358656
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).
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.
Flags: needinfo?(sajid.ahmed)
(comment 37 is assuming mak agrees with me, of course.)
Flags: needinfo?(mak77)
(Reporter)

Comment 39

4 days ago
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.
Flags: needinfo?(sajid.ahmed)
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.