1302855 - Fold browsercomps into xul

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Enes Göktaş [:egoktas]]

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 1

[Enes Göktaş [:egoktas]]

Attachment: Bug 1302855 - Fold browsercomps into xul;

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

Comment 2

[Benjamin Smedberg]

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?

Comment 3

[Enes Göktaş [:egoktas]]

Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78808/diff/1-2/

Comment 4

[Enes Göktaş [:egoktas]]

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 5

[Enes Göktaş [:egoktas]]

Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78808/diff/2-3/

Comment 6

[Enes Göktaş [:egoktas]]

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());

Comment 7

[Enes Göktaş [:egoktas]]

Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78808/diff/3-4/

Comment 8

[Benjamin Smedberg]

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 9

[Benjamin Smedberg]

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();

Comment 10

[Enes Göktaş [:egoktas]]

Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78808/diff/4-5/

Comment 11

[Enes Göktaş [:egoktas]]

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.

Comment 12

[Enes Göktaş [:egoktas]]

Comment on attachment 8791386 [details]
Bug 1302855 - Fold browsercomps into xul;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78808/diff/5-6/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4006b190b344
Fold browsercomps into xul; r=bsmedberg

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4006b190b344