771090 - crash in RtlpCallQueryRegistryRoutine | BaseThreadInitThunk with Flash in webapprt (caused by incorrect plugin-container launch)

Status: VERIFIED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Description

[Andrew Williamson [:eviljeff]]

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 1

[Jason Smith [:jsmith]]

Just got a crash as well with that app, but the crash report looks bizarre:

https://crash-stats.mozilla.com/report/index/bp-9b7d436c-7852-46e9-a539-afd902120705

Formal reports:

https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2012-07-05&signature=RtlQueryRegistryValues%20|%20BaseThreadInitThunk&version=WebappRuntime%3A16.0a1

Comment 2

[Jason Smith [:jsmith]]

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 3

[Jason Smith [:jsmith]]

Appears to happen on Firefox too:

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=RtlQueryRegistryValues%20|%20BaseThreadInitThunk&reason_type=contains&date=07%2F05%2F2012%2015%3A48%3A08&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=RtlQueryRegistryValues%20|%20BaseThreadInitThunk

It's a windows-only crash.

Comment 4

[Andrew Williamson [:eviljeff]]

When I tried in Nightly proper I didn't experience the crash.

Comment 5

[Benjamin Smedberg]

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]

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

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

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

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

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

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

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

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

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

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b579feaa4fc

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b579feaa4fc

Comment 19

[Andrew Williamson [:eviljeff]]

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

Also tested on Mac to check to see if there were any regressions. No regressions detected.

Comment 21

[Jason Smith [:jsmith]]

Also tested on Win XP - no issues detected

Comment 22

[Alex Keybl [:akeybl]]

WebAppRT isn't shipping till FF16, and this is fixed there, so we're good to go here.