656331 - XPCOM binary component registration ignores Module::kVersion

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Alex Vincent [:WeirdAl]]

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]

Attachment: Actually do the version compare, rev. 1

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 531663 [details] [diff] [review]
Actually do the version compare, rev. 1

r=me

Comment 3

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/1706e681390c

Comment 4

[Johnathan Nightingale [:johnath]]

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]]

Affects Fx4 but extremely unlikely we'll ship an extra release for this. Fx5 is coming soon.

Comment 6

[Alex Vincent [:WeirdAl]]

If no one beats me to it, I can work on the testcase tonight, using Mozilla-only code.

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: testcase as patch

Comment 8

[Benjamin Smedberg]

Comment on attachment 531837 [details] [diff] [review]
testcase as patch

brilliantly simple

Comment 9

[(not currently active) Ted Mielczarek]

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]]

Check-in needed for trunk.  Per comment 4, this is a prerequisite for FF5 Aurora.

Comment 11

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/b23a920b981e

Comment 12

[Johnathan Nightingale [:johnath]]

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]]

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 14

[Alex Vincent [:WeirdAl]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/036380520767
http://hg.mozilla.org/releases/mozilla-aurora/rev/bada2f37fecd

Comment 15

[Alex ViR]

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]

No, this is intentionally ==. Binary components must be recompiled for each release per our new policy.

Comment 17

[Alex ViR]

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]]

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

[Alex ViR]

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

[(not currently active) Ted Mielczarek]

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

[Alex ViR]

"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

[(not currently active) Ted Mielczarek]

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]]

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]

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.