Closed Bug 1330529 Opened 3 years ago Closed 2 years ago

Drop SetDllDirectory calls for supporting Shockwave Player

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: emk, Assigned: cpeterson)

References

Details

Attachments

(6 files, 1 obsolete file)

We don't support Shockwave Player anymore.
This Shockwave Player workaround was added in bug 607832.
Blocks: 607832
No longer blocks: npapi-eol
Depends on: npapi-eol
Priority: -- → P3
Assignee: nobody → cpeterson
I removed the unnecessary ShouldProtectPluginCurrentDirectory function and more unnecessary code just kept unraveling... :)
Comment on attachment 8951530 [details]
Bug 1330529 - Part 2: Remove unused SetDllDirectory workaround for Shockwave Player plugin.

https://reviewboard.mozilla.org/r/220826/#review228478
Attachment #8951530 - Flags: review?(jmathies) → review+
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.
Attachment #8951531 - Flags: review?(jmathies) → review+
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!
Attachment #8951532 - Flags: review?(jmathies) → review+
Comment on attachment 8951533 [details]
Bug 1330529 - Part 5: Remove some Windows line endings from PluginProcessChild.cpp.

https://reviewboard.mozilla.org/r/220832/#review228626
Attachment #8951533 - Flags: review?(jmathies) → review+
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
Attachment #8951534 - Flags: review?(jmathies) → review+
(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.
Pushed by cpeterson@mozilla.com:
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
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
(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.
Assignee: cpeterson → VYV03354
Status: NEW → ASSIGNED
Assignee: VYV03354 → cpeterson
Attachment #8953717 - Attachment description: Followup to incorrect file path handling in PDFiumEngineShim → Followup to fix incorrect file path handling in PDFiumEngineShim
See Also: → 1440886
Attachment #8953717 - Attachment is obsolete: true
Attachment #8953717 - Flags: review?(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)?
Attachment #8953759 - Flags: review?(cpeterson) → review+
(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.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb01e88f0d6f
Followup to fix incorrect file path handling in PDFiumEngineShim. r=cpeterson
You need to log in before you can comment on or make changes to this bug.