Closed Bug 750480 Opened 8 years ago Closed 7 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox17 + verified
firefox18 + verified

People

(Reporter: keeler, Assigned: gfritzsche)

References

Details

(Keywords: hang)

Attachments

(3 files, 5 obsolete files)

Attached file testcase (obsolete) —
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.
Severity: normal → critical
Keywords: hang, testcase
OS: Linux → All
Hardware: x86_64 → All
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
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!
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?
(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.
(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.
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.
Checked the status of this issue on a current nightly:
* JRE 6u32 - still occuring
* JRE 6u35 - not occuring
* JRE 7u7  - not occuring
(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> ...
Also:
* JRE 6u34 - not occuring
So it hangs only in blocked Java versions.
Should we close it as WONTFIX?
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.
(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.
Attached file testcase v2
The specific cases we can recover from without always denying access to the plugins scriptable object.
Attachment #619677 - Attachment is obsolete: true
Attached patch Workaround v1 (WIP) (obsolete) — Splinter Review
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)
Attached patch Workaround, v2 (obsolete) — Splinter Review
* 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)
Attached patch Workaround, v3 (obsolete) — Splinter Review
Whitespace fix for above patch.
Attachment #668279 - Attachment is obsolete: true
Attachment #668279 - Flags: review?(benjamin)
Attachment #668407 - Flags: review?(benjamin)
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+
Attached patch Workaround, v4 (obsolete) — Splinter Review
* 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
Attached patch Workaround, v5Splinter Review
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
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?
Keywords: testcasecheckin-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
(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
Closed: 7 years ago
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.
Keywords: checkin-needed
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.
(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.
QA Contact: simona.marcu
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
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.