Closed Bug 1086977 Opened 5 years ago Closed 5 years ago

[10.10] Facebook's old "Facebook Photo Uploader" (fbplugin) crashes on load on OS X Yosemite

Categories

(Core :: Plug-ins, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- wontfix

People

(Reporter: smichaud, Assigned: smichaud)

Details

(Keywords: crash, topcrash-mac, topcrash-thunderbird)

Crash Data

Attachments

(3 files, 6 obsolete files)

This bug's signature is a topcrasher on both Firefox and Thunderbird.  All the crash reports involve an "fbplugin" crashing on Yosemite as soon as it's loaded, here:

https://hg.mozilla.org/mozilla-central/annotate/15099ba111e8/dom/plugins/base/nsPluginsDirDarwin.cpp#l334

This is almost certainly not our bug.

It's already been reported here:
http://forums.mozillazine.org/viewtopic.php?f=39&t=2881031&p=13831205

As best I can tell, "fbplugin" is Facebook's old "Facebook Photo Uploader", documented here:
https://www.facebook.com/note.php?note_id=178492968919

It appears to have been abandoned by Facebook, and is no longer available for download.
The following page (among others) ties "fbplugin" to the "Facebook Photo Updater":
http://www.ehow.com/how_8575837_remove-facebook-photo-uploader.html
In both Firefox and Thunderbird, "fbplugin" is loaded from JavaScript code, and into the main process.  So whether or not it's an NPAPI plugin, it's not loaded as NPAPI plugins normally are (into a separate process).
Atos translation of this crash's "signature" (as called directly from fbplugin):

CFBasicHashSetValue (in CoreFoundation) + 1734
The following quote from https://www.facebook.com/note.php?note_id=178492968919 indicates there's a "Facebook Plug-In" that exists separately from the "Facebook Photo Uploader":

"When you use the new photo uploader for the first time, you'll be asked to install the Facebook Plug-In."

I still haven't been able to find out anything about it, or whether it's still supported and available.
See the comment at http://hg.mozilla.org/releases/mozilla-release/annotate/1f22a8cc7aa5/dom/plugins/base/nsPluginsDirDarwin.cpp#l472

On Mac/Linux, even when we load plugins out of process, we actually load the library in-process first, just to call the function to get the list of MIME types.

We have the option to add this plugin to the blocklist, but it looks like we can't blocklist it only for Yosemite+; we'd have to block it for all mac users. We could also hardcode a block into the plugin loading code specifically for this case. I think we should clearly do that.

Fortunately we know some people at FB: I'll reach out to see if this plugin is supported at all, or is completely dead.
> On Mac/Linux, even when we load plugins out of process, we actually
> load the library in-process first, just to call the function to get
> the list of MIME types.

I stand	corrected.  I agree that's what's going on here.
I'm going to wait to do anything here until we've heard from Facebook.

If we *don't* hear from Facebook in a reasonable time, we'll need to patch this in two stages:

1) Land a patch that records fbplugin-specific info (like version numbers) in crash reports (like we already do for Flash).
2) Once we know which versions crash on load on Yosemite (if they don't all crash), we'll know which versions to block.
Attached patch Fix (obsolete) — Splinter Review
This *should* work.  But since we can't download (or find) "fbplugin", we'll have to land it to find out for sure.

It's really too bad we know so little about "fbplugin".  If we knew more, we might be able to provide more criteria for this block.  But from several hours of Google searching, there don't seem to be any other plugins whose filename is "fbplugin".
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8509850 - Flags: review?(benjamin)
I should have added that I've received email from Facebook that they no longer support "fbplugin" -- any versions thereof.
But yet another email from Facebook throws doubt on the plugin's filename -- it may not be "fbplugin".  It's definitely "fbplugin" that crashes on load, but that executable may be pulled in by the actual plugin when *it's* loaded.

So I should probably add code to my patch to record information (in crash reports) about the plugin that triggers these "fbplugin" crashes-on-load.
Attachment #8509850 - Flags: review?(benjamin)
This fix *might* work.  But if it doesn't, it will record information in crash logs that should allow us to fix these crashes properly.
Attachment #8509850 - Attachment is obsolete: true
Attachment #8510699 - Flags: review?(benjamin)
Comment on attachment 8510699 [details] [diff] [review]
Possible fix plus information for crashlogs

> It might also be the name of a bundle executable,

Oops, this is false.  I'll remove it on landing, or on my next revision.
Comment on attachment 8510699 [details] [diff] [review]
Possible fix plus information for crashlogs

Oops again.  Another thing I need to do is guarantee that the crash log information will be recorded only once (per process).

New patch coming up.
Attachment #8510699 - Flags: review?(benjamin)
I've started a set of tryserver builds:
https://tbpl.mozilla.org/?tree=Try&rev=5b7c4568aaad
Attachment #8510699 - Attachment is obsolete: true
Attachment #8510799 - Flags: review?(benjamin)
Comment on attachment 8510799 [details] [diff] [review]
Possible fix plus information for crashlogs

are you sure that `strstr(path.get(), "fbplugin")` is going to find this? I think it would be simpler to annotate the name of the plugin we're loading in every case, e.g.:

#ifdef MOZ_CRASHREPORTER
CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("PluginMimeinfoLoading"), fileName);
#endif

Then if we keep seeing this crash, we'll know the file name we need to block.
Attachment #8510799 - Flags: review?(benjamin) → review-
Oops, forget my previous comment.  It has the same problem your suggestion does.

I think we should land my patch exactly as it stands.

> are you sure that `strstr(path.get(), "fbplugin")` is going to find this?

No I'm not 100% certain.  But the removal advice given at the link from comment #1 makes me reasonably sure.

Without some kind of check like that, crash reports for unrelated reasons will get littered with irrelevant annotations.

(Too bad we can't remove the last annotation if a crash doesn't happen.  But there doesn't appear to be any support for that.)
Attachment #8510799 - Flags: review- → review?(benjamin)
Comment on attachment 8510799 [details] [diff] [review]
Possible fix plus information for crashlogs

The code here is really complex for what it's doing. For the measurement part, just always do the annotation and clear it when we're finished with the load (set it back to ""). Extra annotations, especially if they are temporary, won't harm us.

According to FHR, here are the plugins that exist in the wild:

Plugin names found: Facebook Desktop,Facebook Video Calling Plugin,ETK FaceBook Plugin,Facebook Photo Uploader,Facebook Video Calling Plugin ,Facebook Plugin

But if I filter by OS==Darwin, the only plugin name that is found is "Facebook Video Calling Plugin".

Do we know that this is or isn't the video-calling plugin? Let's either only land measurement code, or let's land the simplest possible block based entirely on the existing fileName or `info.fName` variables.
Attachment #8510799 - Flags: review?(benjamin) → review-
> For the measurement part, just always do the annotation and clear it
> when we're finished with the load (set it back to "").

I didn't realize we could to this.  If it works, of course it changes the whole game.  But I need to test it to convince myself of that.
> Do we know that this is or isn't the video-calling plugin?

We know only what's documented here, in this bug.  Facebook doesn't seem to be able to tell us anything at all.  And I haven't found anything more definite than what I've already commented on above.
OK, how's this?

As best I can tell, you can't get rid of an annotation, only change it.  In that case, this is probably the best way to avoid confusion.

I decided it wasn't worth the trouble to try to get a partial path to the plugin.  What evidence we have suggests it's not a bundle.
Attachment #8510799 - Attachment is obsolete: true
Attachment #8511307 - Flags: review?(benjamin)
Comment on attachment 8511307 [details] [diff] [review]
Possible fix plus information for crashlogs

>diff --git a/dom/plugins/base/nsPluginsDirDarwin.cpp b/dom/plugins/base/nsPluginsDirDarwin.cpp

>+  // Don't load "fbplugin" (a Facebook plugin) if we're running on OS X 10.10
>+  // (Yosemite) or later.  It crashes on load, in the call to LoadPlugin()
>+  // below.  See bug 1086977.
>+  if (nsCocoaFeatures::OnYosemiteOrLater()) {
>+    if (!strcmp(fileName.get(), "fbplugin")) {

nit, fileName.EqualsLiteral("fbplugin").

r=me with that fixed
Attachment #8511307 - Flags: review?(benjamin) → review+
What I'll land.  Carrying forward r+.
Attachment #8511307 - Attachment is obsolete: true
Attachment #8511337 - Flags: review+
Comment on attachment 8511337 [details] [diff] [review]
Possible fix plus information for crashlogs

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8219d3ab06a1
> As best I can tell, you can't get rid of an annotation, only change it.

For what it's worth, I've opened bug 1088938.
https://hg.mozilla.org/mozilla-central/rev/8219d3ab06a1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
This broke --disable-crashreporter - pushed fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2832e9468c7
Oops :-(  Thanks, Jonathan!
Carrying forward r+.
Attachment #8512911 - Flags: review+
I was about to ask that my patch be uplifted to aurora and beta, as I hadn't yet seen any of these crashes in builds containing my patch.  But just now I looked again and found this:

bp-09735d07-e27f-4967-94b7-64f6b2141027

You need special permissions to see the annotation, which is as follows (in the *.json part of the rawdump):

"Bug%201086977": "fbplugin_1_0_1.plugin"

So at least some of the time the plugin name is fbplugin_1_0_1.plugin.

I suggest that, in addition to plugins whose filename is "fbplugin", we also block all plugins whose name starts with "fbplugin_".  I'll post a new patch shortly.
Attachment #8512911 - Attachment is obsolete: true
Attachment #8512974 - Flags: review?(benjamin)
Attachment #8512974 - Flags: review?(benjamin) → review+
Comment on attachment 8512974 [details] [diff] [review]
Also block plugins named "fbplugin_*"

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/679819e8236f
Weird, it built fine for me on OS X.  I guess I'll have to stop using nsPrintfCString.

New patch coming up.
I've got what I think is a working patch.  But I won't post it until I've had a chance to run it through the tryservers ... which are currently closed.
This should work.  But let's see how it does on the tryservers.

https://tbpl.mozilla.org/?tree=Try&rev=a4ec23de9ca4
Attachment #8512974 - Attachment is obsolete: true
Flags: needinfo?(smichaud)
Comment on attachment 8513125 [details] [diff] [review]
Also block plugins named "fbplugin_*"

This did very well on the tryservers.
Attachment #8513125 - Flags: review?(benjamin)
Attachment #8513125 - Flags: review?(benjamin) → review+
Comment on attachment 8513125 [details] [diff] [review]
Also block plugins named "fbplugin_*"

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76386e21d733
These crashes have now disappeared on the 36 branch.  And there haven't been any reports of problems.

But there are still a lot of these on the 35 and 34 branches.  It's actually the #1 Mac topcrasher on the 35 branch, and it's #15 on the 34 branch.

So I'll create a unified patch suitable for landing on the 35 and 34 branches, and request uplift there.
Carrying forward r+.
Attachment #8521517 - Flags: review+
Comment on attachment 8521517 [details] [diff] [review]
Patch for aurora and beta

Approval Request Comment
[Feature/regressing bug #]: Old Facebook plugin crashes on Yosemite
[User impact if declined]: These crashes will remain a Mac topcrasher on Yosemite
[Describe test coverage new/current, TBPL]: Baked on m-c for two weeks with no problems reported
[Risks and why]: Low risk.  Blocks obsolete plugin on Yosemite.
[String/UUID change made/needed]: None
Attachment #8521517 - Flags: approval-mozilla-beta?
Attachment #8521517 - Flags: approval-mozilla-aurora?
Attachment #8521517 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8521517 [details] [diff] [review]
Patch for aurora and beta

Esr31 is also effected.  Forgot to ask approval earlier.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: These crashes will remain a Mac topcrasher on Yosemite
Fix Landed on Version: 36
Risk to taking this patch (and alternatives if risky): Low risk.  Blocks obsolete plugin on Yosemite.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8521517 - Flags: approval-mozilla-esr31?
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Users of this plugin *always* crash on Yosemite when it's used -- sometimes on startup.  The fix is very simple and straightforward.
Comment on attachment 8521517 [details] [diff] [review]
Patch for aurora and beta

Given the severity of the bug (crash every time the plugin is loaded), that the fix has been verified on Nightly over the past week, and that the fix is pretty simple, I'm going to approve this for beta9. I'm not sure this is worth taking on ESR as we have a large number of issues that we will likely need to backport to properly support Yosemite on ESR31.
Attachment #8521517 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8521517 [details] [diff] [review]
Patch for aurora and beta

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

Based on the feedback in Comment 48 were going to decline this for ESR31.
Attachment #8521517 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31-
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #48)
> Comment on attachment 8521517 [details] [diff] [review]
> Patch for aurora and beta
> 
> Given the severity of the bug (crash every time the plugin is loaded), that
> the fix has been verified on Nightly over the past week, and that the fix is
> pretty simple, I'm going to approve this for beta9. I'm not sure this is
> worth taking on ESR as we have a large number of issues that we will likely
> need to backport to properly support Yosemite on ESR31.

#1 crash for Thunderbird 31.3.0 Mac users, by a wide margin. Most of the users I encounter with this crash don't realize they have the plugin, and for whatever reason, safe mode does not help. So I think at best most users will have fair difficulty in solving their crash.

Do we really need to officially support Yosemite to say it's worth fixing a user topcrash for ESR?
Also, version 24 esr is no longer supported.

For all the above reasons I think we should (re)consider taking this for esr31.  I believe it will be a great help.
There is a big difference between "properly support Yosemite on ESR31" and preventing a "#1 crash for Thunderbird 31.3.0 Mac users, by a wide margin" I agree with Wayne, let's land this on esr31. Risk is minimal.
You need to log in before you can comment on or make changes to this bug.