Closed Bug 1317726 Opened 3 years ago Closed 3 years ago

sdp_file_parser still depends upon xpcom glue

Categories

(Core :: WebRTC: Signaling, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(1 file)

I chose the wrong moz.build template when making this a standalone binary and it *still* depends upon xpcom glue.

It looks like the only symbol it is using is NS_DebugBreak.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Rank: 17
Priority: -- → P1
@Dan: if it helps this does not even need to be gtest or whatever. I only made this as a clone of the sdp_unitests because it was the easiest path. Feel free to remove as much of the dependencies. The only functionality it needs is to pass the content of a file to the SDP parser.
Maybe you can also try to fix bug 1317764 at the same time :-)
Blocks: 1317764
Comment on attachment 8811301 [details]
Bug 1317726 - sdp_file_parser should not depend on xpcom glue;

https://reviewboard.mozilla.org/r/93450/#review93548

::: media/webrtc/signaling/src/sdp/sipcc/cpr_string.c:13
(Diff revision 1)
> -#include "mozilla/mozalloc.h"
> -
> -#define cpr_malloc(a) moz_xmalloc(a)
> +#define cpr_malloc(a) malloc(a)
> +#define cpr_calloc(a, b) calloc(a, b)
> +#define cpr_realloc(a, b) realloc(a, b)
> -#define cpr_calloc(a, b) moz_xcalloc(a, b)
> -#define cpr_realloc(a, b) moz_xrealloc(a, b)

I'm not sure what the effect of this change is on Firefox itself.
Would it be possible to only re-define the moz_x*() function to the standard libc function inside the sdp_file_parser itself instead, so that Firefox would still keep using the moz_x* versions?
(In reply to Nils Ohlmeier [:drno] from comment #5)
> Comment on attachment 8811301 [details]
> Bug 1317726 - sdp_file_parser should not depend on xpcom glue;
> 
> https://reviewboard.mozilla.org/r/93450/#review93548
> 
> ::: media/webrtc/signaling/src/sdp/sipcc/cpr_string.c:13
> (Diff revision 1)
> > -#include "mozilla/mozalloc.h"
> > -
> > -#define cpr_malloc(a) moz_xmalloc(a)
> > +#define cpr_malloc(a) malloc(a)
> > +#define cpr_calloc(a, b) calloc(a, b)
> > +#define cpr_realloc(a, b) realloc(a, b)
> > -#define cpr_calloc(a, b) moz_xcalloc(a, b)
> > -#define cpr_realloc(a, b) moz_xrealloc(a, b)
> 
> I'm not sure what the effect of this change is on Firefox itself.
> Would it be possible to only re-define the moz_x*() function to the standard
> libc function inside the sdp_file_parser itself instead, so that Firefox
> would still keep using the moz_x* versions?

Will do!

(My understanding is that these would cause Firefox to use the fallible allocator, which would mean that the results of each call need to be checked to see if they are null before being used. I double checked, and this isn't done consistently in this file and so this change isn't safe. I had initially thought that this would still use the moz_x* calls behind the scenes.)
Comment on attachment 8811301 [details]
Bug 1317726 - sdp_file_parser should not depend on xpcom glue;

https://reviewboard.mozilla.org/r/93450/#review94040
Attachment #8811301 - Flags: review?(drno) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a36e60d1c4c
sdp_file_parser should not depend on xpcom glue; r=drno
https://hg.mozilla.org/mozilla-central/rev/3a36e60d1c4c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.