Closed Bug 1302855 Opened 8 years ago Closed 8 years ago

Fold browsercomps into xul

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: egoktas, Assigned: egoktas)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

CFI (with cross library support) in Clang 3.8.1 requires the cfi-compiled libraries to be loaded at the initialization of the process. However, because libbrowsercomps.so ends up in a subdirectory, this library cannot be found, unless you set LD_LIBRARY_PATH. Folding browsercomps into xul would solve the not-found problem and it will not require LD_LIBRARY_PATH.
Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

https://reviewboard.mozilla.org/r/78808/#review77570

::: browser/components/feeds/nsFeedSniffer.cpp:192
(Diff revision 1)
>  {
> -  int32_t offset = dataString.Find(substring);
> -  if (offset == -1)
> +  nsACString::const_iterator start, end;
> +  dataString.BeginReading(start);
> +  dataString.EndReading(end);
> +
> +  if(!FindInReadable(nsCString(substring), start, end)){

This is overcomplicated:

1) nsDependentCString(substring) avoid the allocation of nsCString.
2) you don't need the iterators: FindInReadable(nsDependentCString(substring), dataString) does the same job.

::: browser/components/feeds/nsFeedSniffer.cpp:198
(Diff revision 1)
>      return false;
> +  }
> +
> +  int32_t offset = (int32_t) (start.get() - dataString.Data());
>  
> +  //const char *begin = dataString.Data();

Don't leave commented code.

::: browser/components/feeds/nsFeedSniffer.cpp:326
(Diff revision 1)
>    if (!isFeed) {
> +    nsACString::const_iterator start, end;
> +
> +    dataString.BeginReading(start);
> +    dataString.EndReading(end);
> +    bool foundNS_RDF = FindInReadable(nsCString(NS_RDF), start, end);

Again avoid the iterators. Also NS_LITERAL_CSTRING rather than nsCString, to avoid allocation.

::: browser/components/shell/nsGNOMEShellService.cpp:73
(Diff revision 1)
>  };
>  
>  // GConf registry key constants
>  #define DG_BACKGROUND "/desktop/gnome/background"
>  
> -static const char kDesktopImageKey[] = DG_BACKGROUND "/picture_filename";
> +#define kDesktopImageKey DG_BACKGROUND "/picture_filename"

Why was this change necessary? Does NS_LITERAL_CSTRING(literalArray) not compile correctly?

::: browser/components/shell/nsGNOMEShellService.cpp:511
(Diff revision 1)
>  
>  static void
>  ColorToCString(uint32_t aColor, nsCString& aResult)
>  {
>    // The #rrrrggggbbbb format is used to match gdk_color_to_string()
> -  char *buf = aResult.BeginWriting(13);
> +  char *buf = aResult.BeginWriting();

This is a functional change that is likely to cause memory corruption. BeginWriting(13) sets the string capacity to 13 characters. BeginWriting() doesn't.

So this needs to use SetLength(13) to match the prior behavior.

::: widget/nsBaseWidget.cpp:613
(Diff revision 1)
>  // Add a child to the list of children
>  //
>  //-------------------------------------------------------------------------
>  void nsBaseWidget::AddChild(nsIWidget* aChild)
>  {
> -  MOZ_RELEASE_ASSERT(!aChild->GetNextSibling() && !aChild->GetPrevSibling(),
> +  MOZ_ASSERT(!aChild->GetNextSibling() && !aChild->GetPrevSibling(),

unrelated change?
Attachment #8791386 - Flags: review?(benjamin) → review-
Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

https://reviewboard.mozilla.org/r/78808/#review77570

> Why was this change necessary? Does NS_LITERAL_CSTRING(literalArray) not compile correctly?

Yep I was having the following errors:

/home/enes/projects/apps/ff/mozilla-central-clean/browser/components/shell/nsGNOMEShellService.cpp:428:26: error: expected ')'
      NS_LITERAL_CSTRING(kDesktopBGSchema), getter_AddRefs(background_settings));
                         ^
/home/enes/projects/apps/ff/mozilla-central-clean/browser/components/shell/nsGNOMEShellService.cpp:428:7: note: to match this '('
      NS_LITERAL_CSTRING(kDesktopBGSchema), getter_AddRefs(background_settings));
      ^
/home/enes/projects/apps/ff/mozilla-central-clean/obj-ff/dist/include/nsLiteralString.h:33:104: note: expanded from macro 'NS_LITERAL_CSTRING'
#define NS_LITERAL_CSTRING(s)                     static_cast<const nsLiteralCString&>(nsLiteralCString("" s))
                                                                                                       ^
/home/enes/projects/apps/ff/mozilla-central-clean/browser/components/shell/nsGNOMEShellService.cpp:434:57: error: expected ')'
      background_settings->SetString(NS_LITERAL_CSTRING(kDesktopOptionGSKey),
                                                        ^
/home/enes/projects/apps/ff/mozilla-central-clean/browser/components/shell/nsGNOMEShellService.cpp:434:38: note: to match this '('
      background_settings->SetString(NS_LITERAL_CSTRING(kDesktopOptionGSKey),
                                     ^

> This is a functional change that is likely to cause memory corruption. BeginWriting(13) sets the string capacity to 13 characters. BeginWriting() doesn't.
> 
> So this needs to use SetLength(13) to match the prior behavior.

Do we need to copy aResult before setting length?

> unrelated change?

I think this was in your browsercomps-win.patch.
But I am reverting it.
Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

https://reviewboard.mozilla.org/r/78808/#review77570

> This is overcomplicated:
> 
> 1) nsDependentCString(substring) avoid the allocation of nsCString.
> 2) you don't need the iterators: FindInReadable(nsDependentCString(substring), dataString) does the same job.

How could I get the offset of substring in dataString without iterators?
After finding the substring, I was computing the offset by getting the difference between the iterator position (start) and the dataString address.

int32_t offset = (int32_t) (start.get() - dataString.Data());
Blocks: 1302891
Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

https://reviewboard.mozilla.org/r/78808/#review77570

> Yep I was having the following errors:
> 
> /home/enes/projects/apps/ff/mozilla-central-clean/browser/components/shell/nsGNOMEShellService.cpp:428:26: error: expected ')'
>       NS_LITERAL_CSTRING(kDesktopBGSchema), getter_AddRefs(background_settings));
>                          ^
> /home/enes/projects/apps/ff/mozilla-central-clean/browser/components/shell/nsGNOMEShellService.cpp:428:7: note: to match this '('
>       NS_LITERAL_CSTRING(kDesktopBGSchema), getter_AddRefs(background_settings));
>       ^
> /home/enes/projects/apps/ff/mozilla-central-clean/obj-ff/dist/include/nsLiteralString.h:33:104: note: expanded from macro 'NS_LITERAL_CSTRING'
> #define NS_LITERAL_CSTRING(s)                     static_cast<const nsLiteralCString&>(nsLiteralCString("" s))
>                                                                                                        ^
> /home/enes/projects/apps/ff/mozilla-central-clean/browser/components/shell/nsGNOMEShellService.cpp:434:57: error: expected ')'
>       background_settings->SetString(NS_LITERAL_CSTRING(kDesktopOptionGSKey),
>                                                         ^
> /home/enes/projects/apps/ff/mozilla-central-clean/browser/components/shell/nsGNOMEShellService.cpp:434:38: note: to match this '('
>       background_settings->SetString(NS_LITERAL_CSTRING(kDesktopOptionGSKey),
>                                      ^

Interesting! This is a result of bug 1155963. I guess this is ok, given that history; it surprises me.

> Do we need to copy aResult before setting length?

I don't know what "copy" means here. You're assigning to aResult as an outparam, so any data that was already there is irreleveant.
Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

https://reviewboard.mozilla.org/r/78808/#review78538

::: browser/components/build/moz.build:1
(Diff revision 4)
>  # -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-

This patch needs to modify CLOBBER, or it will break inbound.

::: browser/components/feeds/nsFeedSniffer.cpp:192
(Diff revision 4)
>  {
> -  int32_t offset = dataString.Find(substring);
> -  if (offset == -1)
> +  nsACString::const_iterator start, end;
> +  dataString.BeginReading(start);
> +  dataString.EndReading(end);
> +
> +  if(!FindInReadable(nsCString(substring), start, end)){

nit, unsnuggle the if (

::: browser/components/feeds/nsFeedSniffer.cpp:196
(Diff revision 4)
> +
> +  if(!FindInReadable(nsCString(substring), start, end)){
>      return false;
> +  }
> +
> +  int32_t offset = (int32_t) (start.get() - dataString.Data());

Is the int32_t cast necessary? Could this just be:

 auto offset = start.get() - dataString.Data();
Attachment #8791386 - Flags: review?(benjamin) → review+
Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

https://reviewboard.mozilla.org/r/78808/#review77570

> Interesting! This is a result of bug 1155963. I guess this is ok, given that history; it surprises me.

Ok leaving in the #defines.

> I don't know what "copy" means here. You're assigning to aResult as an outparam, so any data that was already there is irreleveant.

Ok got it. I think that I thought aResult had relevant data.
https://hg.mozilla.org/mozilla-central/rev/4006b190b344
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1308327
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 52 → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: