Last Comment Bug 656331 - XPCOM binary component registration ignores Module::kVersion
: XPCOM binary component registration ignores Module::kVersion
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla5
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-11 09:36 PDT by Alex Vincent [:WeirdAl]
Modified: 2011-08-15 06:48 PDT (History)
10 users (show)
benjamin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
-
wanted


Attachments
Actually do the version compare, rev. 1 (1.72 KB, patch)
2011-05-11 10:03 PDT, Benjamin Smedberg [:bsmedberg]
bzbarsky: review+
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
testcase as patch (6.52 KB, patch)
2011-05-11 20:36 PDT, Alex Vincent [:WeirdAl]
benjamin: review+
Details | Diff | Splinter Review

Description Alex Vincent [:WeirdAl] 2011-05-11 09:36:40 PDT
At APN, we compiled a binary component for FF4, using an old XULRunner SDK.  Module::kVersion was 1, and with the FF5 Aurora code changing that to 2, we expected we needed to recompile for FF5.

However, in smoke-testing, I discovered that XPCOM code is not checking the Module::kVersion field of the component it is registering.  This means that a binary component compiled for one version of XPCOM (including an invalid version like 220) would be enabled across all versions of XPCOM from Gecko 2.0+.

I recognize that Module::kVersion was changed for a reason, to force binary components to update.  Allowing mismatched kVersion fields means an incompatible DLL could register - or worse, supersede a properly compiled DLL for that version of Firefox (see bug 616056 for details).
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-05-11 10:03:57 PDT
Created attachment 531663 [details] [diff] [review]
Actually do the version compare, rev. 1
Comment 2 Boris Zbarsky [:bz] 2011-05-11 10:07:11 PDT
Comment on attachment 531663 [details] [diff] [review]
Actually do the version compare, rev. 1

r=me
Comment 3 Benjamin Smedberg [:bsmedberg] 2011-05-11 10:47:57 PDT
http://hg.mozilla.org/mozilla-central/rev/1706e681390c
Comment 4 Johnathan Nightingale [:johnath] 2011-05-11 11:48:54 PDT
Discussed in triage today - we'd like to approve this, but given that we've missed it before, it needs a test.
Comment 5 Daniel Veditz [:dveditz] 2011-05-11 11:49:50 PDT
Affects Fx4 but extremely unlikely we'll ship an extra release for this. Fx5 is coming soon.
Comment 6 Alex Vincent [:WeirdAl] 2011-05-11 14:42:47 PDT
If no one beats me to it, I can work on the testcase tonight, using Mozilla-only code.
Comment 7 Alex Vincent [:WeirdAl] 2011-05-11 20:36:07 PDT
Created attachment 531837 [details] [diff] [review]
testcase as patch
Comment 8 Benjamin Smedberg [:bsmedberg] 2011-05-12 06:33:38 PDT
Comment on attachment 531837 [details] [diff] [review]
testcase as patch

brilliantly simple
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-05-12 06:51:48 PDT
Totally nitpicky, but our modern Makefile style is two-space indent after line continuations, like:
EXTRA_DSO_LDOPTS = \
  $(NSPR_LIBS) \
...
  $(NULL)
Comment 10 Alex Vincent [:WeirdAl] 2011-05-12 07:49:58 PDT
Check-in needed for trunk.  Per comment 4, this is a prerequisite for FF5 Aurora.
Comment 11 Benjamin Smedberg [:bsmedberg] 2011-05-12 08:14:31 PDT
http://hg.mozilla.org/mozilla-central/rev/b23a920b981e
Comment 12 Johnathan Nightingale [:johnath] 2011-05-12 08:50:10 PDT
Comment on attachment 531663 [details] [diff] [review]
Actually do the version compare, rev. 1

a=me on this patch and the accompanying test once it runs clean on central. Thanks for the quick turnaround!
Comment 13 JP Rosevear [:jpr] 2011-05-12 14:45:21 PDT
Please land for Aurora by Monday May 16 or the approval will potentially be lost.  Please mark as status-firefox5 fixed when you do.
Comment 15 alexvir666@yahoo.com 2011-06-17 02:52:41 PDT
There is a bug in /xpcom/components/nsNativeComponentLoader.cpp!
>> if (mozilla::Module::kVersion == data.module->mVersion && ...

I think there should be "<=" instead of "==", cause:
* in Firefox7a - kVersion=7
* in Firefox6a - kVersion=6
* in Firefox5b - kVersion=2

For example XPCOM component built with old SDK supports Firefox7a with kVersion=7. If this component will be running under Firefox5b with the fix I wrote above - all will be fine:
  mozilla::Module::kVersion = 2;
  data.module->mVersion = 7;
  2 <= 7 == OK
Comment 16 Benjamin Smedberg [:bsmedberg] 2011-06-17 06:13:33 PDT
No, this is intentionally ==. Binary components must be recompiled for each release per our new policy.
Comment 17 alexvir666@yahoo.com 2011-06-21 05:34:40 PDT
So you think that I should have for example 3 XPI files with different XPCOM files for 3 different versions of Firefox??? Or should I include 3 different XPCOM files (with similar source code, but different kVersion value in Module unit) in one XPI file? What's a reason for this? I think Firefox must have backwards compatibility with older versions.
Comment 18 Alex Vincent [:WeirdAl] 2011-06-21 07:04:53 PDT
It's a pain point, to be sure (particularly when waiting for a SDK weeks after it should have been posted, grumble, grumble), but it's actually a defensive decision on Mozilla's part.

Simply put, the interfaces (and all code) in FF4+ are never frozen.  So let's say you're relying on a XPCOM service in FF5.  In FF7, someone removes that service.  If you QI for that service, and then try to use it, that usually results in a null-dereference crash for binaries.

Also, this also frees Mozilla up to make compatibility-breaking changes.  Mozilla's had to maintain backwards-compatibility support for over 10 years on its first XPCOM model; that's pretty difficult to do, and means bad architectural decisions made 10 years ago have to be supported in perpetuity.  Can you say decisions you're making now should hold up for 10 years or more?  I can't - my crystal ball tends to break after 10 months.

Finally, do recall that binary components are a risk point for Mozilla:  they can cause crashes, memory leaks, and other instabilities.  Mozilla has very few ways to respond to a buggy DLL (and some of them quite draconian).  They're harder to maintain than JS-based components, generally speaking, and this forces them to be much more actively maintained than JS-based components.

---

Having to ship a new DLL every six weeks, per the new release schedule, is painful... and in my opinion, it should be.  Binaries are dangerous, much more so than JavaScript-based components.  You really should use binary components only as a last resort, and only to support the specific capabilities you need.

There are alternatives which I haven't explored yet (js-ctypes, the ipccode of bug 68702), which should be considered as well.  In theory, these alternatives would mean you ship a JS-based component which could access binary code without the XPCOM boilerplate... and that means shipping a single DLL.  In practice... I need code samples.

My apologies to everyone for the spammy bug post.
Comment 19 alexvir666@yahoo.com 2011-06-21 23:32:09 PDT
I understand all that you wrote. But for example my XPCOM dll has only 1 class which doesn't use any other FF interfaces. It just allows to connect from JS-code (through the methods of my own interface/class) with my stanalone EXE application to obtain some info & vice versa - my EXE can connect to JS-code.

P.S. If someone will delete any service from FF7 for example - I think you'll got the NULL as a result when you query that service. Am I right?
Comment 20 Ted Mielczarek [:ted.mielczarek] 2011-06-22 06:35:30 PDT
It sounds like you'd be better served using js-ctypes then, which will let you call your C++ code from JS without any XPCOM in between, and without a need to recompile for each Firefox release:
https://developer.mozilla.org/en/js-ctypes/Using_js-ctypes
Comment 21 alexvir666@yahoo.com 2011-06-22 06:54:50 PDT
"js-ctypes" is not usable for me, cause 1st of all from my JS-code I need to know where the DLL is located. 2nd - it's too difficult to use js-ctypes functions, in XPCOM I have scriptable interface which allows to use "in" & "out" params in a simplest way.
Comment 22 Ted Mielczarek [:ted.mielczarek] 2011-06-22 07:00:39 PDT
These are both solvable problems, but this discussion should move to a newsgroup, not a bug:
http://www.mozilla.org/about/forums/#dev-platform
or
http://www.mozilla.org/about/forums/#dev-extensions

In any event, you are welcome to continue using binary XPCOM components, you will just have to recompile them for every version. That's the trade-off, sorry.
Comment 23 AndreiD[QA] 2011-08-12 08:46:30 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

I was trying to see if this is fixed for Fx6 since the flag "status-firefox6" is set on "fixed", but I couldn't.
Is there a test case or any steps / guidelines for this bug that can be used to verify the fix? Thanks
Comment 24 Benjamin Smedberg [:bsmedberg] 2011-08-15 06:48:42 PDT
Find a binary component compiled against an earlier version and try to register it in Firefox 6? I doubt this needs additional verification, though, since there has been plenty of discussion about it in the groups.

Note You need to log in before you can comment on or make changes to this bug.