Closed Bug 874167 Opened 6 years ago Closed 4 years ago

Use OOPP for Java on Windows (again)

Categories

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

x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla43
Iteration:
37.2
Tracking Status
e10s m7+ ---
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: benjamin, Assigned: jimm)

References

(Blocks 1 open bug, )

Details

Attachments

(5 files, 2 obsolete files)

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.
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?
Priority: -- → P2
Aaron, do you think setting hangUITimeoutSecs models the proposed changes sufficiently for testing?
Assignee: nobody → georg.fritzsche
Flags: needinfo?(aklotz)
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.
Flags: needinfo?(aklotz)
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)
Keywords: qawanted
(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.
Keywords: qawanted
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: p=0
@jimm - this is the bug we talked about for java OOP
tracking-e10s: --- → ?
Blocks: e10s-plugins
Unassigning for now to reflect the current status, but added to our prioritization list.
Assignee: georg.fritzsche → nobody
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.
Depends on: 1099299
No longer depends on: 1099299
Duplicate of this bug: 1099299
time to do this for e10s only.
Duplicate of this bug: 1084153
Duplicate of this bug: 1101430
Attached patch pref patch (obsolete) — Splinter Review
We need to test java way before m7, so lets get this turned back on for e10s on Windows.
Assignee: nobody → jmathies
Attachment #8535618 - Flags: review?(wmccloskey)
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?
Attachment #8535618 - Flags: review?(wmccloskey) → review+
Comment on attachment 8535618 [details] [diff] [review]
pref patch

oops, sorry
Attachment #8535618 - Attachment is obsolete: true
Attached patch patch (r=billm) (obsolete) — Splinter Review
Attached patch patch (r=billm)Splinter Review
- whitespace cleanup
Attachment #8535707 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/38116e28a7b8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Iteration: --- → 37.2
Flags: qe-verify?
Whiteboard: p=0
Was the infinite timeout implemented? Will the security signing dialog crash after 45-sec?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
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
Attachment #8648099 - Flags: review?(jmathies)
Blocks: 1194780
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!
Attachment #8648099 - Flags: review?(jmathies) → review+
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 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
Attachment #8648099 - Attachment description: , → , r=jimm
Bug 1090864 part A - remove nsNPAPIPlugin::RunPluginOOP and unused code paths, r=jimm
Attachment #8648248 - Flags: review?(jmathies)
Bug 1098064 part B - remove nsIPluginHost.isPluginOOP, r?jimm
Attachment #8648249 - Flags: review?(jmathies)
Bug 1098064 part C - remove SimpleTest and reftest testPluginIsOOP and related usage, r=jimm
Attachment #8648250 - Flags: review?(jmathies)
(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 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.
Attachment #8648248 - Flags: review?(jmathies) → review+
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!
Attachment #8648249 - Flags: review?(jmathies) → review+
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!
Attachment #8648250 - Flags: review?(jmathies) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
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
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla37 → mozilla43
Summary: Use OOPP for Java on Windows (again), with infinite plugin timeout if the hang UI shows → Use OOPP for Java on Windows (again)
Looks like the commits in comment 36 landed in bug 1194780.
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
Attachment #8648099 - Attachment description: 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, → 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,
Attachment #8648099 - Flags: approval-mozilla-aurora?
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.
Attachment #8648099 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer depends on: 1196115
You need to log in before you can comment on or make changes to this bug.