597316 - overwrite wmode in object parameters when plugins.force.wmode is set

Status: RESOLVED WONTFIX

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

Description

[Roger WANG]

Attachment: overwrite wmode parameter

plugins.force.wmode doesn't work in some cases: the flash video near the top left corner of http://video.sina.com.cn doesn't show in fennec.

There're 2 places specifying the wmode parameter in the webpage and they need to be overwritten properly.

Comment 1

[Josh Aas]

Comment on attachment 476171 [details] [diff] [review]
overwrite wmode parameter

We add a wmode attribute if the pref forces it, is the issue that the web page is also specifying attributes and those are getting read instead of our forced entry?

Comment 2

[Roger WANG]

(In reply to comment #1)

> We add a wmode attribute if the pref forces it, is the issue that the web page
> is also specifying attributes and those are getting read instead of our forced
> entry?

yes

Comment 3

[Josh Aas]

Comment on attachment 476171 [details] [diff] [review]
overwrite wmode parameter

Looks good, just compare the result of "strcmp" to "0" instead of using "!" - the latter incorrectly implies that we're testing a boolean value.

Comment 4

[Roger WANG]

Attachment: patch v2

thanks, here is the updated patch.

( will it lost the previous r+? )

Comment 5

[Josh Aas]

Comment on attachment 476503 [details] [diff] [review]
patch v2

>+    if (!wmodeType.IsEmpty() &&
>+        !strcmp(mCachedAttrParamNames[nextAttrParamIndex], "wmode") == 0) {

You forgot to remove the "!" before strcmp.

Comment 6

[Roger WANG]

Attachment: patch v3

oops, my bad, update again.

Comment 7

[Josh Aas]

Comment on attachment 476506 [details] [diff] [review]
patch v3

>+        ToNewUTF8String(NS_ConvertUTF8toUTF16(wmodeType));

Why are you starting with a UTF8 string, converting it to UTF16, and then converting it back to UTF8?

Comment 8

[Roger WANG]

Attachment: patch v4

Not sure on this. It was written like that in the unpatched version in line ~3671, and it was copied.

update it since it's not necessary.

Comment 9

[Josh Aas]

Comment on attachment 476577 [details] [diff] [review]
patch v4

Looks good, I'd also like to have jst look at this since we've had problems with this code before.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 476577 [details] [diff] [review]
patch v4

 
     mCachedAttrParamNames [nextAttrParamIndex] = ToNewUTF8String(name);
     mCachedAttrParamValues[nextAttrParamIndex] = ToNewUTF8String(value);
+    if (!wmodeType.IsEmpty() &&
+        strcmp(mCachedAttrParamNames[nextAttrParamIndex], "wmode") == 0) {
+      mCachedAttrParamValues[nextAttrParamIndex] = ToNewCString(wmodeType);
+    }

This will overwrite the old value in mCachedAttrParamValues[nextAttrParamIndex], which we allocated above, and thus will now leak.

@@ -3731,6 +3735,10 @@
     value.Trim(" \n\r\t\b", PR_TRUE, PR_TRUE, PR_FALSE);
     mCachedAttrParamNames [nextAttrParamIndex] = ToNewUTF8String(name);
     mCachedAttrParamValues[nextAttrParamIndex] = ToNewUTF8String(value);
+    if (!wmodeType.IsEmpty() &&
+        strcmp(mCachedAttrParamNames[nextAttrParamIndex], "wmode") == 0) {
+      mCachedAttrParamValues[nextAttrParamIndex] = ToNewCString(wmodeType);
+    }

Same thing here.

sr- due to the memory leaks.

Comment 11

[Roger WANG]

Attachment: patch v5

Thanks for pointing out that. Patch is updated. Please have a review.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 476984 [details] [diff] [review]
patch v5

     mCachedAttrParamNames [nextAttrParamIndex] = ToNewUTF8String(name);
     mCachedAttrParamValues[nextAttrParamIndex] = ToNewUTF8String(value);
+    if (!wmodeType.IsEmpty() &&
+        strcmp(mCachedAttrParamNames[nextAttrParamIndex], "wmode") == 0) {
+      NS_Free(mCachedAttrParamValues[nextAttrParamIndex]);
+      mCachedAttrParamValues[nextAttrParamIndex] = ToNewCString(wmodeType);
+    }

Much better, but would it not be better yet to change this to something along the lines of:

     mCachedAttrParamNames [nextAttrParamIndex] = ToNewUTF8String(name);
     if (!wmodeType.IsEmpty() &&
         strcmp(mCachedAttrParamNames[nextAttrParamIndex], "wmode") == 0) {
       mCachedAttrParamValues[nextAttrParamIndex] = ToNewCString(wmodeType);
     } else {
       mCachedAttrParamValues[nextAttrParamIndex] = ToNewUTF8String(value);
     }
Less allocator traffic, less code...?

Comment 13

[Roger WANG]

Attachment: patch v6

of course. thanks

Comment 14

[Roger WANG]

Comment on attachment 476991 [details] [diff] [review]
patch v6

sorry, there is an error in it

Comment 15

[Roger WANG]

Attachment: patch v7

this is the correct patch. thanks

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 476994 [details] [diff] [review]
patch v7

Looks good, thanks for fixing this!

Comment 17

[Roger WANG]

proposed for fenenc 2.0b1 since it will affect flash on many sites

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 476994 [details] [diff] [review]
patch v7

are there any tests for this?

Comment 19

[Roger WANG]

(In reply to comment #18)
> Comment on attachment 476994 [details] [diff] [review]
> patch v7
> 
> are there any tests for this?

I think not, and I'll write one.

btw, should the test be ready before the check-in of this patch?

TIA.

Comment 20

[Doug Turner (:dougt)]

Yes.  Test and then approval and then push.  Thanks!

Comment 21

[Johnny Stenback (:jst)]

Roger, any news on the tests for this? :)

Comment 22

[Roger WANG]

(In reply to comment #21)
> Roger, any news on the tests for this? :)

not yet. Just start figuring out how to write test for it after a long PRC national holiday :)

Comment 23

[Roger WANG]

I'm trying to set the pref in the same way as layout/generic/test/test_bug263683.html, but it always failed with:

###!!! ASSERTION: cannot set pref from content process: 'Error', file /home/wwang29/mozilla-central/modules/libpref/src/nsPrefBranch.cpp, line 191

Note that when I try to run the case layout/generic/test/test_bug263683.html it's also fails with the same error.

Is it a limitation for those cases?

Comment 24

[Roger WANG]

Attachment: test case

firefox is not IPC enabled, so setting the param works.

attached is the case, please review, thanks.

Comment 25

[Lokesh Goel]

Adding checkin-needed.

Comment 26

[(no longer active)]

Please wait for the test to be reviewed as well.  This can be marked as checkin-needed then.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 483068 [details] [diff] [review]
test case

Sorry this review request fell through the cracks :(. r=jst, assuming this still passes.

Comment 28

[BMO Automation]

Resolving as wont fix, plugin support deprecated in Firefox 85.