Show a console warning when using deprecated mozIStorageBaseStatement methods

RESOLVED FIXED in Firefox 55

Status

()

defect
P2
normal
RESOLVED FIXED
5 years ago
2 years ago

People

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

Tracking

({addon-compat})

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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.
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)
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
Whiteboard: [good next bug][mentor=mak][lang=cpp] → [good next bug][lang=cpp]
Comment hidden (obsolete)
Comment hidden (obsolete)
Priority: -- → P2
Whiteboard: [good next bug][lang=cpp] → [good first bug][lang=cpp]
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Comment 8

2 years 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.
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 years 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.
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 years ago
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)
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+
note that in a debug build MOZ_ASSERT will crash.
(Assignee)

Comment 16

2 years 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.
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 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+
(Assignee)

Comment 20

2 years 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.
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

2 years ago
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)
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+
Try looks ok, the failures are unrelated, even if it was an unlucky push!
We can land, once you attach the final patch.
Assignee: nobody → sajid.ahmed
Status: NEW → ASSIGNED
(Assignee)

Comment 25

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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2f7bd2c69db
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 years 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.)
(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.
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...
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++.
I'd be fine with doing that, if it helps. I'll file a bug to add it to the deprecation warning.
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)
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.