Closed Bug 1385115 Opened 7 years ago Closed 7 years ago

dom/media/gmp/GMPChild.cpp:438:33: error: 'XUL_LIB_FILE' was not declared in this scope

Categories

(Core :: Audio/Video: GMP, defect, P1)

Unspecified
FreeBSD
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

(Keywords: regression)

Attachments

(1 file)

In file included from objdir/dom/media/gmp/Unified_cpp_dom_media_gmp0.cpp:2:
dom/media/gmp/ChromiumCDMAdapter.cpp:180:5: error: use of undeclared identifier 'close'
    close(mFile);
    ^
In file included from objdir/dom/media/gmp/Unified_cpp_dom_media_gmp0.cpp:65:
dom/media/gmp/GMPChild.cpp:438:33: error: use of undeclared identifier 'XUL_LIB_FILE'
      NS_SUCCEEDED(path->Append(XUL_LIB_FILE)) && FileExists(path) &&
                                ^
2 errors generated.
The regression is caused by mozilla-central changeset eb898389180a. Can you adjust "Blocks" field?
Flags: needinfo?(cpearce)
(In reply to Jan Beich from comment #1)
> The regression is caused by mozilla-central changeset eb898389180a. Can you
> adjust "Blocks" field?

Certainly.

The code in eb898389180a won't have any practical effect on FreeBSD, so I'd be happy so to take a patch to ifdef it out on non Mac/Windows/Linux builds.
Flags: needinfo?(cpearce)
Component: DOM → Audio/Video: GMP
Rank: 17
Priority: -- → P1
(In reply to Chris Pearce (:cpearce) from comment #2)
> ifdef it out on non Mac/Windows/Linux builds

GMP itself (not just CDM) is a liability on Tier3 due to lack of sandboxing. Without knowing the context and being able to test I'd prefer to keep the statu quo. Adding --disable-gmp is preferable[1] but is also more work.

[1] Tor Browser can probably take advantage of it:
    https://trac.torproject.org/projects/tor/ticket/15910
    https://trac.torproject.org/projects/tor/ticket/16285
Comment on attachment 8891315 [details]
Bug 1385115 - Unbreak build on Tier3 after bug 1382883.

https://reviewboard.mozilla.org/r/162522/#review167780

::: dom/media/gmp/GMPChild.cpp:351
(Diff revision 1)
>  #define FIREFOX_FILE NS_LITERAL_STRING("firefox")
>  #define XUL_LIB_FILE NS_LITERAL_STRING("XUL")
> -#elif defined(XP_LINUX)
> -#define FIREFOX_FILE NS_LITERAL_STRING("firefox")
> -#define XUL_LIB_FILE NS_LITERAL_STRING("libxul.so")
>  #elif defined(OS_WIN)

This is weird. `OS_*` come from Chromium. Gecko shouldn't use them outside of IPC and WebRTC.
Do we support platforms where XP_WIN != OS_WIN or XP_UNIX != OS_POSIX?
Flags: needinfo?(cpearce)
Comment on attachment 8891315 [details]
Bug 1385115 - Unbreak build on Tier3 after bug 1382883.

https://reviewboard.mozilla.org/r/162522/#review168542

r+, provided you please update the patch to fix the OS_WIN/XP_WIN issue you pointed out. Thanks for the patch! Sorry for breaking tier3!

::: dom/media/gmp/GMPChild.cpp:351
(Diff revision 1)
>  #define FIREFOX_FILE NS_LITERAL_STRING("firefox")
>  #define XUL_LIB_FILE NS_LITERAL_STRING("XUL")
> -#elif defined(XP_LINUX)
> -#define FIREFOX_FILE NS_LITERAL_STRING("firefox")
> -#define XUL_LIB_FILE NS_LITERAL_STRING("libxul.so")
>  #elif defined(OS_WIN)

Yeah, that should be XP_WIN, and not OS_WIN. I must have copypasta'd that from somewhere.

Please change that to XP_WIN.
Attachment #8891315 - Flags: review?(cpearce) → review+
(In reply to Jan Beich from comment #6)
> Do we support platforms where XP_WIN != OS_WIN or XP_UNIX != OS_POSIX?

I don't believe so.

I recall now I had copied OS_POSIX from Chromium's content_decryption_module.h in a previous version of my patch in bug 1382883, and that didn't work, and now you've explained why. That explains how that got there.
Flags: needinfo?(cpearce)
Comment on attachment 8891315 [details]
Bug 1385115 - Unbreak build on Tier3 after bug 1382883.

https://reviewboard.mozilla.org/r/162522/#review168542

OK. Fixed but only `OS_WIN` coming from bug 1382883 to allow backporting to FF56 if this bug misses merge date. Other instances of `OS_*` to be fixed in a separate bug.
Keywords: checkin-needed
See Also: → 1386200
https://hg.mozilla.org/mozilla-central/rev/9408284f094a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee: nobody → jbeich
You need to log in before you can comment on or make changes to this bug.