Last Comment Bug 771090 - crash in RtlpCallQueryRegistryRoutine | BaseThreadInitThunk with Flash in webapprt (caused by incorrect plugin-container launch)
: crash in RtlpCallQueryRegistryRoutine | BaseThreadInitThunk with Flash in web...
Status: VERIFIED FIXED
[blocking-webrtdesktop1+], [appreview...
: crash, reproducible
Product: Firefox Graveyard
Classification: Graveyard
Component: Webapp Runtime (show other bugs)
: Trunk
: All Windows 7
: P1 critical
: Firefox 16
Assigned To: Tim Abraldes [:TimAbraldes] [:tabraldes]
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: 769721
  Show dependency treegraph
 
Reported: 2012-07-05 04:08 PDT by Andrew Williamson [:eviljeff]
Modified: 2016-03-21 12:39 PDT (History)
10 users (show)
ryanvm: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (2.07 KB, patch)
2012-07-06 16:40 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
benjamin: review+
Details | Diff | Splinter Review

Description Andrew Williamson [:eviljeff] 2012-07-05 04:08:29 PDT
Apps that use flash appear to be causing the runtime to crash after a few seconds (triggering the crash reporter).  I tried a few apps that worked and the common theme for ones that crash is flash.

To confirm my suspicion I hacked webapp.json to have about:addons as the origin and disabled the flash plugin, then reverted webapp.json.  The app no longer crashed.

Test case:
https://marketplace.mozilla.org/en-US/app/calcudoku
(the app has flash adverts)

I only noticed this in the 2012-07-04 Nightly.
Comment 2 Jason Smith [:jsmith] 2012-07-05 08:45:07 PDT
Anthony or Ben - Something landed recently for flash signatures on nightly, right? Could these crashes be related to that? Any ideas on what this is?
Comment 4 Andrew Williamson [:eviljeff] 2012-07-05 08:52:09 PDT
When I tried in Nightly proper I didn't experience the crash.
Comment 5 Benjamin Smedberg [:bsmedberg] 2012-07-05 09:08:15 PDT
The relevant possible bugs are:

* bug 769048 - Report on crashes from the Flash runtime
* bug 769721 - Force-enable OOPP for Flash users on Windows Vista+ 

Given the symptoms here, it seems very likely that bug 769721 is the cause of this behavior. I suspect that webapprt incorrectly was defaulting to run plugins in-process before (because the preferences are stored in firefox.js, not all.js). There may also be a bug with OOPP on Windows where we're looking in the wrong directory for plugin-container.exe or something like that.

This is probably due to incorrect code at http://hg.mozilla.org/mozilla-central/annotate/ee2c5f2928b6/ipc/glue/GeckoChildProcessHost.cpp#l108

The non-windows codepath uses NS_GRE_DIR correctly (probably because FF-on-XR on Linux forced somebody to fix this bug already!), but the windows codepath seems to assume that plugin-container will always be in the same directory as the launched executable, which is obviously incorrect for webapprt. So that's where this probably needs to be fixed.
Comment 6 Benjamin Smedberg [:bsmedberg] 2012-07-05 09:15:31 PDT
This blocks webapprt which is currently enabled for aurora(15), so we need to track this one way or another.
Comment 7 Mike Hommey [:glandium] 2012-07-05 09:21:22 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> The non-windows codepath uses NS_GRE_DIR correctly (probably because
> FF-on-XR on Linux forced somebody to fix this bug already!)

Looks like it.
http://hg.mozilla.org/mozilla-central/rev/0b615190e79f
Comment 8 Jason Smith [:jsmith] 2012-07-05 22:40:02 PDT
Really easy ways to reproduce this bug:

1. Use the app Andy provided
2. Use the app here - http://pearce.org.nz/fullscreen/ and run the plugins
Comment 9 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-07-06 16:40:32 PDT
Created attachment 639848 [details] [diff] [review]
Patch v1

The attached patch seems to do the right thing on Windows, and I *think* I've left the codepath for other OSes unmodified.  However, I don't have machines with other OSes on which I can build and run this patch.

This patch is running through tryserver:
  https://tbpl.mozilla.org/?tree=Try&rev=a018963ab58f
Comment 10 Jason Smith [:jsmith] 2012-07-07 12:48:37 PDT
(In reply to Tim Abraldes from comment #9)
> Created attachment 639848 [details] [diff] [review]
> Patch v1
> 
> The attached patch seems to do the right thing on Windows, and I *think*
> I've left the codepath for other OSes unmodified.  However, I don't have
> machines with other OSes on which I can build and run this patch.
> 
> This patch is running through tryserver:
>   https://tbpl.mozilla.org/?tree=Try&rev=a018963ab58f

Tested this patch with the test cases in comment 8 and bundlr. No crashes detected. No obvious regressions either.
Comment 11 Benjamin Smedberg [:bsmedberg] 2012-07-10 07:05:33 PDT
Also (perhaps in another bug) we should make webapprt use the same plugin prefs as Firefox:
http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js?mark=898-911#898
Comment 12 Andrew Williamson [:eviljeff] 2012-07-10 07:31:10 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> Also (perhaps in another bug) we should make webapprt use the same plugin
> prefs as Firefox:
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> js?mark=898-911#898

IMO, its not that straightforward.  e.g. Prefs are profile specific - which profile?  If the prefs are copied would changing the prefs in the profile after installing the app be reflected?  What happens if the Firefox profile used is deleted?
Comment 13 Benjamin Smedberg [:bsmedberg] 2012-07-10 07:44:26 PDT
This is a default pref file, and is not related to a particular profile at all. I'm pretty sure that the equivalent webapprt file is http://mxr.mozilla.org/mozilla-central/source/webapprt/prefs.js
Comment 14 Jason Smith [:jsmith] 2012-07-10 07:45:05 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> Also (perhaps in another bug) we should make webapprt use the same plugin
> prefs as Firefox:
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> js?mark=898-911#898

Agreed. I wonder if that might fix bug 749792 by caveat if we use those default prefs. Maybe add it to bug 749792?
Comment 15 Jason Smith [:jsmith] 2012-07-10 07:49:39 PDT
(In reply to Jason Smith [:jsmith] from comment #14)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> > Also (perhaps in another bug) we should make webapprt use the same plugin
> > prefs as Firefox:
> > http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> > js?mark=898-911#898
> 
> Agreed. I wonder if that might fix bug 749792 by caveat if we use those
> default prefs. Maybe add it to bug 749792?

I've also filed bug 772447 (which might be a better place to put this) for reviewing the default prefs for desktop firefox and implementing ones that should be the web runtime, but are missing.
Comment 16 Andrew Williamson [:eviljeff] 2012-07-10 07:50:56 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> This is a default pref file, and is not related to a particular profile at
> all. I'm pretty sure that the equivalent webapprt file is
> http://mxr.mozilla.org/mozilla-central/source/webapprt/prefs.js

Oh, okay.  I misunderstood.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:35:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b579feaa4fc
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-07-10 20:31:20 PDT
https://hg.mozilla.org/mozilla-central/rev/3b579feaa4fc
Comment 19 Andrew Williamson [:eviljeff] 2012-07-11 07:20:17 PDT
Works for me in the latest Nightly, both for the app I originally mentioned (Calcudoku) and another app that was crashing (qb)
Comment 20 Jason Smith [:jsmith] 2012-07-11 09:49:45 PDT
Also tested on Mac to check to see if there were any regressions. No regressions detected.
Comment 21 Jason Smith [:jsmith] 2012-07-11 10:49:57 PDT
Also tested on Win XP - no issues detected
Comment 22 Alex Keybl [:akeybl] 2012-07-16 13:51:02 PDT
WebAppRT isn't shipping till FF16, and this is fixed there, so we're good to go here.

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