1202024 - Plugin Quirks modes don't get initialised soon enough when doing async init.

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

See bug 1182411 comment 33.

Comment 1

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac75de739e02

Comment 2

[Bob Owen (:bobowen)]

Attachment: MozReview Request: Bug 1202024: Initialize plugin details and quirks on first run for async init. r=aklotz

Bug 1202024: Initialize plugin details and quirks on first run for async init. r?aklotz

Comment 3

[Bob Owen (:bobowen)]

This change means that the Telemetry call won't happen for async init, but as this was happening on the second use of the plugin anyway, I'm not sure it was what we wanted.

If we need the Telemetry for async init, it should probably be somewhere else.

Also, the plugin details weren't set for the first plugin use, so things like hang and crash data wouldn't have had this.
Might have been causing other issues as I'm not sure what else the data is used for.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

https://reviewboard.mozilla.org/r/18377/#review16667

r=me with issue resolved.

::: dom/plugins/ipc/PluginModuleParent.cpp:2551
(Diff revision 1)
> +        InitQuirksModes(nsDependentCString(pluginType));

It seems like we only really need one call site for InitQuirksModes. How about we move this above the if block and then remove the second call to InitQuirksModes below?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8657545 [details]
MozReview Request: Bug 1202024: Initialize plugin details and quirks on first run for async init. r=aklotz

https://reviewboard.mozilla.org/r/18379/#review16671

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

https://reviewboard.mozilla.org/r/18379/#review16675

OK, after chatting with Bob we've sorted out a different solution.

::: dom/plugins/ipc/PluginModuleParent.cpp:2552
(Diff revision 1)
> +

Let's get rid of this since the next issue is going to address it.

::: dom/plugins/ipc/PluginModuleParent.cpp:2568
(Diff revision 1)
>      if (mPluginName.IsEmpty()) {

Move this block to be the first thing in PluginModuleParent::NPP_NewInternal. Then everything should just work (TM). Plus it should fix an existing telemetry bug :-)

Comment 7

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5912d5f9b323

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8657545 [details]
MozReview Request: Bug 1202024: Initialize plugin details and quirks on first run for async init. r=aklotz

Bug 1202024: Initialize plugin details and quirks on first run for async init. r=aklotz

Comment 9

[Bob Owen (:bobowen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8f6186e342570f0853f537c398714be359cbc86
Bug 1202024: Initialize plugin details and quirks in parent on first run for async init. r=aklotz

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8f6186e3425

Comment 11

[Bob Owen (:bobowen)]

Comment on attachment 8657545 [details]
MozReview Request: Bug 1202024: Initialize plugin details and quirks on first run for async init. r=aklotz

Approval Request Comment
[Feature/regressing bug #]:
Bug 1182411

[User impact if declined]:
Flash settings menu will not be available on first run of windowless flash when async init enabled, which it is on Beta.
Also, fixes a bug with the plugin initialisation telemetry for async init.

[Describe test coverage new/current, TreeHerder]:
No flash tests in tree, but original test case and other windowless and windowed flash content tested manually.

[Risks and why]: 
Low to medium: patch is a very simple move of existing code to ensure it is called on first run for async init.  Only adding "to medium" because the NPAPI code is pretty complicated.

[String/UUID change made/needed]:
None

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8657545 [details]
MozReview Request: Bug 1202024: Initialize plugin details and quirks on first run for async init. r=aklotz

We have decided to disable async plugin init for FF41 release, please see bug 1204036. Given that decision, this patch does not meet the bar for (beta) 41 RC uplift.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8657545 [details]
MozReview Request: Bug 1202024: Initialize plugin details and quirks on first run for async init. r=aklotz

Taking it for 42.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef38f6fcac87

Comment 15

[Petruta Horea [:phorea], Desktop QA]

What tests could I use to manually verify this is fixed? Thank You!

The following testcase still causes small hangs with Firefox 42 beta 1:
http://demo.oicweave.org/test-firefox/test_crashed.html

Comment 16

[Bob Owen (:bobowen)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #15)
> What tests could I use to manually verify this is fixed? Thank You!
> 
> The following testcase still causes small hangs with Firefox 42 beta 1:
> http://demo.oicweave.org/test-firefox/test_crashed.html

I think that's probably something to do with what that is testing, but I'd comment on bug 1197803, if what you are seeing is something different.

If you test in 42.0b1 it looks like async init is still on by default, so you can check any windowless, flash content and the right-click menu should work first time round (which it wasn't before).

Comment 17

[Petruta Horea [:phorea], Desktop QA]

Thanks!

Verified as fixed with Firefox 42 beta 6 and Dev Ed 43.0s2 2015-10-13 under Win 7 64-bit.