hang when loading empty java applet with click-to-play

RESOLVED FIXED in Firefox 17

Status

()

--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: keeler, Assigned: gfritzsche)

Tracking

({hang})

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17+ verified, firefox18+ verified)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Created attachment 619677 [details]
testcase

Firefox hangs (have to kill -9 it) when loading a java applet with no actual code when click-to-play is enabled. Also happens on Win7.

Updated

7 years ago
Severity: normal → critical
Keywords: hang, testcase
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

7 years ago
Assignee: nobody → georg.fritzsche
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
I am seeing a hang in the Java plugin here for NPP_GetValue() with variable=NPPVpluginScriptableNPObject.

Logs so far show no difference between the initialization compared to click_to_play=false.
With click_to_play being disabled however, NPP_GetValue() is never called.

Callstack:
  ntdll.dll!_NtWaitForSingleObject@12()  + 0x15 bytes	
  ntdll.dll!_NtWaitForSingleObject@12()  + 0x15 bytes	
  kernel32.dll!_WaitForSingleObjectExImplementation@12()  + 0x43 bytes	
  kernel32.dll!_WaitForSingleObject@8()  + 0x12 bytes	
  npjp2.dll!6daa149f() 	
  [Frames below may be incorrect and/or missing, no symbols loaded for npjp2.dll]	
  jvm.dll!6d8eac3b() 	
  jvm.dll!6d97c3a1() 	
  jvm.dll!6d8eacbd() 	
  jvm.dll!6d885981() 	
  jvm.dll!6d887c6c() 	
  jvm.dll!6d7f2b50() 	
  npjp2.dll!6daa1772() 	
  npjp2.dll!6daa1cb9() 	
  npjp2.dll!6daa29f3() 	
  npjp2.dll!6daa223c() 	
  xul.dll!nsNPAPIPluginInstance::GetValueFromPlugin(NPPVariable variable=862372800, void * value=0x448bc3c0)  Line 611 + 0x55 bytes	C++
 	33660189()
See also bug 708461 and bug 717986.

Hi, Georg!

Comment 3

7 years ago
The hang is only applicable to JRE 6 (I've tried 6u32).
JRE 7u4 (the one available in java.com) doesn't have this issue.

I also noticed that an applet embedded in a html page using the applet tag can't be loaded with Aurora.
Is it expected?
(Assignee)

Comment 4

7 years ago
(In reply to Calvin Cheung from comment #3)
> The hang is only applicable to JRE 6 (I've tried 6u32).
> JRE 7u4 (the one available in java.com) doesn't have this issue.

Thanks Calvin, i am seeing the same here. Do you have any insights on what leads to the hang in JRE 6 so we could avoid or work around it?
 
> I also noticed that an applet embedded in a html page using the applet tag
> can't be loaded with Aurora.
> Is it expected?

I opened bug 753280 on this.

Comment 5

7 years ago
(In reply to Georg Fritzsche from comment #4)
> (In reply to Calvin Cheung from comment #3)
> The hang is only applicable to
> JRE 6 (I've tried 6u32).
> JRE 7u4 (the one available in java.com) doesn't
> have this issue.

Thanks Calvin, i am seeing the same here. Do you have any
> insights on what leads to the hang in JRE 6 so we could avoid or work around
> it?
 
In NPP_GetValue, we're trying to get the scripting object for applet and run a message pump on the browser side. Since the applet is empty, we never get the scripting object and the message pump runs forever. 
We can port part of the fix in JRE 7 to JRE 6 for checking if all necessary params are there before starting the jvm.

Georg,

Is it possible for the FF side not to call the NPP_GetValue for the click-to-play case? As you mentioned, that function wasn't call if click-to-play isn't enabled.

Comment 6

7 years ago
If we were going to work around this bug, how would we know that it's an empty applet and that we shouldn't ask for the scriptable object? It doesn't sound like we have a simple way to work around this bug.

Updated

6 years ago
tracking-firefox17: --- → +
tracking-firefox18: --- → +
(Assignee)

Comment 7

6 years ago
Checked the status of this issue on a current nightly:
* JRE 6u32 - still occuring
* JRE 6u35 - not occuring
* JRE 7u7  - not occuring
(Assignee)

Comment 8

6 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> ...
Also:
* JRE 6u34 - not occuring

Comment 9

6 years ago
So it hangs only in blocked Java versions.
Should we close it as WONTFIX?

Comment 10

6 years ago
Well... the point of CTP short-term is that it will be used to block those older (insecure) versions. That is not a good user experience. What is the smallest workaround we can use? i.e. detect a blank <applet> and avoid asking it for a scriptable prototype.
(Assignee)

Comment 11

6 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> That is not a good user experience. What is the
> smallest workaround we can use? i.e. detect a blank <applet> and avoid
> asking it for a scriptable prototype.

A rough test showed this being doable in nsNPAPIPluginInstance. I'm still checking how to properly catch the "blank applet + specific java-version" there; not sure yet though if we have a way to check wether it was initiated via click-to-play.
(Assignee)

Comment 12

6 years ago
Created attachment 667552 [details]
testcase v2

The specific cases we can recover from without always denying access to the plugins scriptable object.
Attachment #619677 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Created attachment 667560 [details] [diff] [review]
Workaround v1 (WIP)

WIP workaround that does not ask the applet for its scriptable in the discoverable cases for the affected versions.

Turns out there are two separate cases with different affected JRE versions:
 * hang on the parameter "code" missing (up to and including 6u33)
 * hang on the parameter "code" being present but empty (up to and including 6u35)

This is working for me for JRE6 on Windows, but i still need to adress the other platforms tomorrow - confirming there and checking for the versioning scheme incosistencies as per:
https://wiki.mozilla.org/QA/Plugins/Java

Benjamin, can you take a quick look at this wether you agree on the basic approach?
Attachment #667560 - Flags: feedback?(benjamin)
(Assignee)

Comment 14

6 years ago
Created attachment 668279 [details] [diff] [review]
Workaround, v2

* tested working across platforms
* adjusted version check (the Java plugins jip mimetype is a consistent version indicator)
* also catch affected Java 7 versions
Attachment #667560 - Attachment is obsolete: true
Attachment #667560 - Flags: feedback?(benjamin)
Attachment #668279 - Flags: review?(benjamin)
(Assignee)

Comment 15

6 years ago
Created attachment 668407 [details] [diff] [review]
Workaround, v3

Whitespace fix for above patch.
Attachment #668279 - Attachment is obsolete: true
Attachment #668279 - Flags: review?(benjamin)
Attachment #668407 - Flags: review?(benjamin)

Comment 16

6 years ago
Comment on attachment 668407 [details] [diff] [review]
Workaround, v3

>+  for (uint32_t i=0; i<mimeCount; ++i) {

unsnuggle operators: i = 0 i < mimeCount

>+    nsCOMPtr<nsIDOMMimeType> ptr(dont_AddRef(mimeTypes[i]));
>+
>+    if (foundVersion) {
>+      continue;

Why not just early-return and skip "foundVersion" entirely?

>+    type.ReplaceChar('_', '.');
>+    version = NS_ConvertUTF16toUTF8(type);
>+    foundVersion = true;

i.e. here just `return true;`

>+void
>+nsNPAPIPluginInstance::CheckJavaC2PJSObjectQuirk(uint16_t paramCount,
>+                                                 const char* const* paramNames,
>+                                                 const char* const* paramValues)
>+{
>+  if (!mMIMEType ||
>+      !nsPluginHost::IsJavaMIMEType(mMIMEType)) {
>+    return;
>+  }

This isn't an especially good check. Please use nsPluginTag::mIsJavaPlugin for this check instead.

>+  nsCString pluginVersion;
>+  rv = pluginTag->GetVersion(pluginVersion);
>+  if (NS_FAILED(rv)) {
>+    return;
>+  }

You never use this variable. Is it leftover from a previous version?

>+
>+  // Due to the Java version being specified incosistently across platforms

"inconsistently"

>+  if (!haveCodeParam && version>="1.6.0.34" && version<"1.7") {

again unsnuggle operators

r=me with those changes. Re-request review if you're not sure of something.
Attachment #668407 - Flags: review?(benjamin) → review+
(Assignee)

Comment 17

6 years ago
Created attachment 668662 [details] [diff] [review]
Workaround, v4

* switched to retrieving the nsPluginTag instead of nsIPluginTag, so:
  * using nsPluginTag::mIsJavaPlugin now
  * able to use early return in GetJavaVersionFromMimetype() now (couldn't before due to needing to release the nsIDOMMimeType instances)
* adressed mentioned oversights

Try: https://tbpl.mozilla.org/?tree=Try&rev=b725d7b1ebe2
Attachment #668407 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
Created attachment 668703 [details] [diff] [review]
Workaround, v5

Fixed stupid oversight: removed code that was unused after last patchs changes.
Try: https://tbpl.mozilla.org/?tree=Try&rev=97359dac98f9
Attachment #668662 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
Comment on attachment 668703 [details] [diff] [review]
Workaround, v5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play
User impact if declined: Browser hanging on click-to-play activation of empty Java applets
Testing completed (on m-c, etc.): I tested this on Win7, OSX 10.8, & Linux with the relevant Java 6 & 7 versions.
Risk to taking this patch (and alternatives if risky): Low risk as this triggers only for click-to-play activation of Java applets. Worst case scenario is Javascript not getting access to an applets scriptable object after C2P-activation in an unaffected case.
String or UUID changes made by this patch: None.
Attachment #668703 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Keywords: testcase → checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5364c43651c1

Should this be [leave open] for a real fix? Also, should this have a test?
Flags: in-testsuite?
Keywords: checkin-needed
(Assignee)

Comment 21

6 years ago
(In reply to Ryan VanderMeulen from comment #20)
> Should this be [leave open] for a real fix? 

Good question - Benjamin, David, do we want to investigate this further for post-FF17?

> Also, should this have a test?

I don't see how we could test this sensibly as that would require several specific Java versions for the test-enviroments.
Flags: in-testsuite? → in-testsuite-
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/5364c43651c1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox18: --- → fixed
Resolution: --- → FIXED
Comment on attachment 668703 [details] [diff] [review]
Workaround, v5

[Triage Comment]
Please land asap on Aurora to make it into FF17 before the merge.
Attachment #668703 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
$ hg qpush
applying 750480-workaround-5.patch
patching file dom/plugins/base/nsNPAPIPluginInstance.cpp
Hunk #2 FAILED at 171
Hunk #5 FAILED at 1691
2 out of 5 hunks FAILED -- saving rejects to file dom/plugins/base/nsNPAPIPluginInstance.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 750480-workaround-5.patch

I'm not unwrapping .rej's right now.
(Assignee)

Comment 25

6 years ago
Created attachment 669038 [details] [diff] [review]
Workaround for aurora
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/127927ebf7c6
status-firefox17: --- → fixed
Keywords: checkin-needed
Keywords: verifyme
I tried to reproduce the hang with JRE 6u32 but the only issues I see before the fix landed are:
- I can't open any new tabs from "+" button
- the Firefox process remains opened in the Control Panel and I have to kill it from the task manager

Could you please offer me guidance in reproducing this hang? Is this the hang that you are referring to in the Description of this bug? 

The above issues are not reproducible on Firefox 17 beta 5.
(Assignee)

Comment 28

6 years ago
(In reply to Simona B [QA] from comment #27)
> - I can't open any new tabs from "+" button
> - the Firefox process remains opened in the Control Panel and I have to kill
> it from the task manager

As per IRC this is Firefox hanging and thus the issue this bug is about.
Assuming by comment 28 that this can be marked verified fixed for Firefox 17. Please correct me if I am wrong.
status-firefox17: fixed → verified
Verified as fixed on Firefox 18 beta 1 - the hang described in Comment 27 is not reproducible when loading the test case attached in the description with JRE 6u32 (and click to play enabled) on Windows 7 and Ubuntu 12.04:

Mozilla/5.0 (Windows NT 6.1; rv:18.0) Gecko/18.0 Firefox/18.0
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0
status-firefox18: fixed → verified
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.