874167 - Use OOPP for Java on Windows (again)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Benjamin Smedberg]

The only real reason we turned OOPP off on Windows/Java is because Java dialogs such as their security signing dialog don't spin a nested event loop and therefore crash after the hang timeout.

But we are seeing a new increase in crashes caused by Java, and they appear unwilling to fix these without specific steps to reproduce. To protect Firefox, I'd like to propose:

* switch OOPP back on for Java on Windows
* When the hang monitor UI shows, don't even auto-timeout and kill Java; just keep the UI showing until the user explicitly kills Java or the plugin recovers
* If the hang UI is disabled or fails for some reason, kill Java after the normal (45-second) timeout.

Comment 1

[Georg Fritzsche [:gfritzsche]]

How about having QA do a test-run with settings that produce matching behaviour now, so we see what scope this is? 
I think just setting dom.ipc.plugins.hangUITimeoutSecs to a very high value should do it?

Comment 2

[Georg Fritzsche [:gfritzsche]]

Aaron, do you think setting hangUITimeoutSecs models the proposed changes sufficiently for testing?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

To model the proposed changes, you would need to set the value of dom.ipc.plugins.timeoutSecs very high, as that pref still governs auto-timeout even when the hang UI is enabled.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Ah, thanks Aaron.

re qawanted: Could we have some test-runs on Windows with the common test-applets to see if that causes any major issues?
You'd need to set 
* dom.ipc.plugins.java.enabled to true
* dom.ipc.plugins.timeoutSecs to a high value (say 3600)

Comment 5

[Paul Silaghi, QA [:pauly]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> You'd need to set 
> * dom.ipc.plugins.java.enabled to true
> * dom.ipc.plugins.timeoutSecs to a high value (say 3600)
Firefox freezes running several applets:
http://www.walter-fendt.de/ph14e/
nightly 2013-05-22 Win 7 x64
Everything looked ok with the default settings.

Comment 6

[John Schoenick [:johns]]

@jimm - this is the bug we talked about for java OOP

Comment 7

[Georg Fritzsche [:gfritzsche]]

Unassigning for now to reflect the current status, but added to our prioritization list.

Comment 8

[Jim Mathies [:jimm]]

Does anyone have any good old bugs related to this with test cases? afaict java should be running out of process now on windows with e10s.

Comment 10

[Jim Mathies [:jimm]]

time to do this for e10s only.

Comment 13

[Jim Mathies [:jimm]]

Attachment: pref patch

We need to test java way before m7, so lets get this turned back on for e10s on Windows.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8535618 [details] [diff] [review]
pref patch

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

r+ for the changes to nsNPAPIPlugin.cpp. It looks like everything else was for debugging?

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 8535618 [details] [diff] [review]
pref patch

oops, sorry

Comment 16

[Jim Mathies [:jimm]]

Attachment: patch (r=billm)

Comment 17

[Jim Mathies [:jimm]]

Attachment: patch (r=billm)

- whitespace cleanup

Comment 18

[Jim Mathies [:jimm]]

 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4c74ff43913e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38116e28a7b8

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38116e28a7b8

Comment 21

[Masatoshi Kimura [:emk]]

Was the infinite timeout implemented? Will the security signing dialog crash after 45-sec?

Comment 22

[Jim Mathies [:jimm]]

(In reply to Masatoshi Kimura [:emk] from comment #21)
> Was the infinite timeout implemented?

Not currently, the hang dialog is currently disabled (bug 1096664).

> Will the security signing dialog crash
> after 45-sec?

My hope would be that it doesn't lock up with the current configuration. I've been testing with the little java I can find on the web and haven't run into any issues.

Comment 23

[Benjamin Smedberg]

Attachment: MozReview Request: Bug 874167 - Move Java to be out-of-process like other plugins, so that it's weird threading behavior doesn't cause Firefox instability. This will likely cause reoccurance of bug 555699 and that is an acceptable tradeoff at this point,

Bug 874167 - Move Java to be out-of-process like other plugins, so that it's weird threading behavior doesn't cause Firefox instability. This will likely cause reoccurance of bug 555699 and that is an acceptable tradeoff at this point, r=jimm

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8648099 [details]
MozReview Request: Bug 874167 - Move Java to be out-of-process like other plugins, so that it's weird threading behavior doesn't cause Firefox instability. This will likely cause reoccurance of bug 555699 and that is an acceptable tradeoff at this point,

https://reviewboard.mozilla.org/r/16111/#review14447

Ship It!

Comment 25

[Jim Mathies [:jimm]]

I do see an occasional java hang but they get caught by the e10s hang monitor. I haven't seen a chrome hang yet, that might change once we get out to beta.

Comment 26

[Benjamin Smedberg]

Comment on attachment 8648099 [details]
MozReview Request: Bug 874167 - Move Java to be out-of-process like other plugins, so that it's weird threading behavior doesn't cause Firefox instability. This will likely cause reoccurance of bug 555699 and that is an acceptable tradeoff at this point,

Bug 874167 - Move Java to be out-of-process like other plugins, so that it's weird threading behavior doesn't cause Firefox instability. This will likely cause reoccurance of bug 555699 and that is an acceptable tradeoff at this point, r=jimm

Comment 27

[Benjamin Smedberg]

Attachment: MozReview Request: Bug 1090864 part A - remove nsNPAPIPlugin::RunPluginOOP and unused code paths, r=jimm

Bug 1090864 part A - remove nsNPAPIPlugin::RunPluginOOP and unused code paths, r=jimm

Comment 28

[Benjamin Smedberg]

Attachment: MozReview Request: Bug 1098064 part B - remove nsIPluginHost.isPluginOOP, r?jimm

Bug 1098064 part B - remove nsIPluginHost.isPluginOOP, r?jimm

Comment 29

[Benjamin Smedberg]

Attachment: MozReview Request: Bug 1098064 part C - remove SimpleTest and reftest testPluginIsOOP and related usage, r=jimm

Bug 1098064 part C - remove SimpleTest and reftest testPluginIsOOP and related usage, r=jimm

Comment 30

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #25)
> I do see an occasional java hang but they get caught by the e10s hang
> monitor. I haven't seen a chrome hang yet, that might change once we get out
> to beta.

example: https://crash-stats.mozilla.com/report/index/7ef03c41-addb-481c-b00b-0344a2150815

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 8648248 [details]
MozReview Request: Bug 1090864 part A - remove nsNPAPIPlugin::RunPluginOOP and unused code paths, r=jimm

https://reviewboard.mozilla.org/r/16145/#review14499

Ship It!

::: dom/plugins/base/nsNPAPIPlugin.cpp
(Diff revision 1)
> -  useA11yPref =  a11y::Compatibility::IsJAWS();

We might want to flag dbolter on this. Looks like we disable OOP still on XP when we detect jaws in out process.

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 8648249 [details]
MozReview Request: Bug 1098064 part B - remove nsIPluginHost.isPluginOOP, r?jimm

https://reviewboard.mozilla.org/r/16147/#review14501

Ship It!

Comment 33

[Jim Mathies [:jimm]]

Comment on attachment 8648250 [details]
MozReview Request: Bug 1098064 part C - remove SimpleTest and reftest testPluginIsOOP and related usage, r=jimm

https://reviewboard.mozilla.org/r/16149/#review14503

Ship It!

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Jim Mathies [:jimm] from comment #31)
> Comment on attachment 8648248 [details]
> MozReview Request: Bug 1090864 part A - remove nsNPAPIPlugin::RunPluginOOP
> and unused code paths, r=jimm
> 
> https://reviewboard.mozilla.org/r/16145/#review14499
> 
> Ship It!
> 
> ::: dom/plugins/base/nsNPAPIPlugin.cpp
> (Diff revision 1)
> > -  useA11yPref =  a11y::Compatibility::IsJAWS();
> 
> We might want to flag dbolter on this. Looks like we disable OOP still on XP
> when we detect jaws in out process.

We're not (yet) sure how much to worry about this potential intersection.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/103d2a9af2ff

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/103d2a9af2ff

These 3 commits landed with bug numbers that don't seem to fit. Please comment/resolve the appropriate bugs for them (and probably add notes to the other bugs noting that commits landed that were misattributed).
https://hg.mozilla.org/mozilla-central/rev/32e5b68506f4
https://hg.mozilla.org/mozilla-central/rev/e4c9fd404528
https://hg.mozilla.org/mozilla-central/rev/4e8302a104fb

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like the commits in comment 36 landed in bug 1194780.

Comment 38

[Benjamin Smedberg]

Comment on attachment 8648099 [details]
MozReview Request: Bug 874167 - Move Java to be out-of-process like other plugins, so that it's weird threading behavior doesn't cause Firefox instability. This will likely cause reoccurance of bug 555699 and that is an acceptable tradeoff at this point,

Approval Request Comment
[Feature/regressing bug #]: Longstanding Java issue
[User impact if declined]: Java crashes will take down the entire browser. This is more urgent because of more rigorous threading checks in Firefox causing more crashes.
[Describe test coverage new/current, TreeHerder]: Limited manual testing.
[Risks and why]: This will cause a regression for Java applets which show various modal dialogs (including certificate dialogs). We've decided to accept this regression to solve the larger issue.
[String/UUID change made/needed]: None

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8648099 [details]
MozReview Request: Bug 874167 - Move Java to be out-of-process like other plugins, so that it's weird threading behavior doesn't cause Firefox instability. This will likely cause reoccurance of bug 555699 and that is an acceptable tradeoff at this point,

OK, let's take it in 42 then.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e1bb46da439