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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: 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.

Comment 1

[Georg Fritzsche [:gfritzsche]]

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()

Comment 2

[Steven Michaud [:smichaud] (Retired)]

See also bug 708461 and bug 717986.

Hi, Georg!

Comment 3

[Calvin Cheung]

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?

Comment 4

[Georg Fritzsche [:gfritzsche]]

(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

[Calvin Cheung]

(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

[Benjamin Smedberg]

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.

Comment 7

[Georg Fritzsche [:gfritzsche]]

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

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

Comment 9

[Scoobidiver (away)]

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

Comment 10

[Benjamin Smedberg]

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.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: testcase v2

The specific cases we can recover from without always denying access to the plugins scriptable object.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: 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?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Attachment: 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

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Workaround, v3

Whitespace fix for above patch.

Comment 16

[Benjamin Smedberg]

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.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Attachment: 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

Comment 18

[Georg Fritzsche [:gfritzsche]]

Attachment: Workaround, v5

Fixed stupid oversight: removed code that was unused after last patchs changes.
Try: https://tbpl.mozilla.org/?tree=Try&rev=97359dac98f9

Comment 19

[Georg Fritzsche [:gfritzsche]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5364c43651c1

Should this be [leave open] for a real fix? Also, should this have a test?

Comment 21

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5364c43651c1

Comment 23

[Alex Keybl [:akeybl]]

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

[Triage Comment]
Please land asap on Aurora to make it into FF17 before the merge.

Comment 24

[Justin Wood (:Callek)]

$ 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.

Comment 25

[Georg Fritzsche [:gfritzsche]]

Attachment: Workaround for aurora

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/127927ebf7c6

Comment 27

[Simona Badau, Desktop QA]

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.

Comment 28

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 29

[u279076]

Assuming by comment 28 that this can be marked verified fixed for Firefox 17. Please correct me if I am wrong.

Comment 30

[Simona Badau, Desktop QA]

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

Comment 31

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old