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

VERIFIED FIXED in Firefox 42

Status

()

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

41 Branch
mozilla43
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 disabled, firefox42 verified, firefox43 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac75de739e02
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
Created 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
Attachment #8657545 - Flags: review?(aklotz)
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.
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 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
Attachment #8657545 - Flags: review?(aklotz) → review+
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 :-)
Attachment #8657545 - Attachment description: MozReview Request: Bug 1202024: Initialize plugin details and quirks on first run for async init. r?aklotz → MozReview Request: Bug 1202024: Initialize plugin details and quirks on first run for async init. r=aklotz
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
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
https://hg.mozilla.org/mozilla-central/rev/d8f6186e3425
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
Attachment #8657545 - Flags: approval-mozilla-beta?
Attachment #8657545 - Flags: approval-mozilla-aurora?
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.
Attachment #8657545 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox41: affected → disabled
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.
Attachment #8657545 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

3 years ago
Blocks: 1195607
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
Flags: needinfo?(bobowen.code)
(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).
Flags: needinfo?(bobowen.code)
Thanks!

Verified as fixed with Firefox 42 beta 6 and Dev Ed 43.0s2 2015-10-13 under Win 7 64-bit.
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
status-firefox43: fixed → verified
You need to log in before you can comment on or make changes to this bug.