1330529 - Drop SetDllDirectory calls for supporting Shockwave Player

Status: RESOLVED FIXED

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

Description

[Masatoshi Kimura [:emk]]

We don't support Shockwave Player anymore.

Comment 1

[Chris Peterson [:cpeterson]]

This Shockwave Player workaround was added in bug 607832.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1330529 - Part 2: Remove unused SetDllDirectory workaround for Shockwave Player plugin.

This code was added in bug 607832 to work around a Shockwave Player bug where it tries to load some DLLs from the current directory, but the current directory is not the one it expects. We no longer support the Shockwave Player plugin, so this workaround is no longer necessary and we can always call SetDllDirectory("") to remove the current directory from the DLL search path.

Review commit: https://reviewboard.mozilla.org/r/220826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220826/

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1330529 - Part 3: Protect DLL loads in wmain instead of waiting until PluginProcessChild::Init.

Removing #define XRE_DONT_PROTECT_DLL_LOAD from plugin-container.cpp and xpcshell.cpp allows the #included nsWindowsWMain.cpp to protect DLL loads much earlier in the plugin process startup.

Review commit: https://reviewboard.mozilla.org/r/220828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220828/

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1330529 - Part 4: Fold nsSetDllDirectory.h into nsWindowsMain.cpp.

nsSetDllDirectory.h consists of just one function definition, SanitizeEnvironmentVariables, which is now only called from nsWindowsMain.cpp. nsSetDllDirectory.h used to define its namesake NS_SetDllDirectory, but the function was removed in bug 699247.

Also remove some #includes that are no longer necessary.

Review commit: https://reviewboard.mozilla.org/r/220830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220830/

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Bug 1330529 - Part 5: Remove some Windows line endings from PluginProcessChild.cpp.

Review commit: https://reviewboard.mozilla.org/r/220832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220832/

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: Bug 1330529 - Part 1: Fix PDFium gtest by using absolute path to PDFium DLL.

This gtest assumed the current working directory ($OBJDIR/_tests/gtest/) was in the DLL LoadLibrary path. That is no longer true with this bug's patches, so the gtest must specify the absolute path to the PDFium DLL to load it.

Review commit: https://reviewboard.mozilla.org/r/220834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220834/

Comment 7

[Chris Peterson [:cpeterson]]

I removed the unnecessary ShouldProtectPluginCurrentDirectory function and more unnecessary code just kept unraveling... :)

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8951530 [details]
Bug 1330529 - Part 2: Remove unused SetDllDirectory workaround for Shockwave Player plugin.

https://reviewboard.mozilla.org/r/220826/#review228478

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8951531 [details]
Bug 1330529 - Part 3: Protect DLL loads in wmain instead of waiting until PluginProcessChild::Init.

https://reviewboard.mozilla.org/r/220828/#review228622

e should keep an eye out for flash bstage as a result of this. They may have 'adjusted' their code to support our envionment.

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8951532 [details]
Bug 1330529 - Part 4: Fold nsSetDllDirectory.h into nsWindowsMain.cpp.

https://reviewboard.mozilla.org/r/220830/#review228624

Thanks for the cleanup work here Chris!

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8951533 [details]
Bug 1330529 - Part 5: Remove some Windows line endings from PluginProcessChild.cpp.

https://reviewboard.mozilla.org/r/220832/#review228626

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8951534 [details]
Bug 1330529 - Part 1: Fix PDFium gtest by using absolute path to PDFium DLL.

https://reviewboard.mozilla.org/r/220834/#review228628

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Jim Mathies [:jimm] from comment #9)
> We should keep an eye out for flash bstage as a result of this. They may have
> 'adjusted' their code to support our envionment.

Sure thing. The Flash content I tested was able to load.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/875a35f50af2
Part 1: Fix PDFium gtest by using absolute path to PDFium DLL. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7138c4369eb
Part 2: Remove unused SetDllDirectory workaround for Shockwave Player plugin. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b539afa21a7c
Part 3: Protect DLL loads in wmain instead of waiting until PluginProcessChild::Init. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/572f3d9d98f0
Part 4: Fold nsSetDllDirectory.h into nsWindowsMain.cpp. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6329ea89ef8
Part 5: Remove some Windows line endings from PluginProcessChild.cpp. r=jimm

Comment 15

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/875a35f50af2#l1.27
>+      const NS_ConvertUTF16toUTF8 externalDll(pdfiumPath);
Please do not use NS_ConvertUTF16toUTF8. It will break non-ASCII file paths on Windows

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to Jim Mathies [:jimm] from comment #9)
> We should keep an eye out for flash bstage as a result of this. They may have
> 'adjusted' their code to support our envionment.

Note that Shockwave Player != Flash Player. The removed code was already dead since we dropped support for Shockwave Player. So I don't think it breaks anything.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Followup to fix incorrect file path handling in PDFiumEngineShim

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/875a35f50af2
https://hg.mozilla.org/mozilla-central/rev/c7138c4369eb
https://hg.mozilla.org/mozilla-central/rev/b539afa21a7c
https://hg.mozilla.org/mozilla-central/rev/572f3d9d98f0
https://hg.mozilla.org/mozilla-central/rev/a6329ea89ef8

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Followup to fix incorrect file path handling in PDFiumEngineShim

Comment 20

[Chris Peterson [:cpeterson]]

Comment on attachment 8953759 [details] [diff] [review]
Followup to fix incorrect file path handling in PDFiumEngineShim

Review of attachment 8953759 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks for fixing this PDFium case that I missed!

::: widget/windows/PDFiumEngineShim.cpp
@@ +64,5 @@
>  {
>    RefPtr<PDFiumEngineShim> inst = sPDFiumEngineShim;
>    if (!inst) {
>      inst = new PDFiumEngineShim();
> +    if (!inst->Init(NS_LITERAL_STRING("pdfium.dll"))) {

Why do we use the filename "pdfium.dll" here, but "pdfium_ref_x86.dll" and "pdfium_ref_x64.dll" in the TestEMFConversion.cpp gtest and "pepperpdfium.dll" in the Mortar extension (browser/extensions/mortar/host/pdf/bootstrap.js)?

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Chris Peterson [:cpeterson] from comment #20)
> Why do we use the filename "pdfium.dll" here, but "pdfium_ref_x86.dll" and
> "pdfium_ref_x64.dll" in the TestEMFConversion.cpp gtest and
> "pepperpdfium.dll" in the Mortar extension
> (browser/extensions/mortar/host/pdf/bootstrap.js)?

I don't know. I only changed the existing nsCString to NS_LITERAL_STRING.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb01e88f0d6f
Followup to fix incorrect file path handling in PDFiumEngineShim. r=cpeterson

Comment 23

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/cb01e88f0d6f