Closed Bug 1345102 Opened 5 years ago Closed 5 years ago

ipc_channel_posix.cc should use OS defined IOV_MAX macro

Categories

(Core :: IPC, defect)

52 Branch
Unspecified
Other
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: petr.sumbera, Unassigned)

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

I'm updating Solaris bundled FF 45ESR to 52ESR. And I'm seeing following errors on console. I believe this problem with IPC is main reason that FF doesn't work (it works but wouldn't load any page). I know that Solaris is not supported but I would appreceate any hint how to debug this issue.

[Parent 1835] WARNING: pipe error: Message too long: file /builds/psumbera/userland-ff-52/components/desktop/firefox/firefox-52.0/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 685
[Parent 1835] WARNING: pipe error: Message too long: file /builds/psumbera/userland-ff-52/components/desktop/firefox/firefox-52.0/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 685
..
Component: Untriaged → IPC
OS: Unspecified → Other
Product: Firefox → Core
The issue seems to be that kMaxIOVecSize is set to 256. Where OS defines max allowed values in IOV_MAX. And Solaris set it as low as 16.
Summary: [Parent 1835] WARNING: pipe error: Message too long: ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 685 → ipc_channel_posix.cc should use OS defined IOV_MAX macro
Attached patch Bug1345102.patch (obsolete) — Splinter Review
Attachment #8848147 - Flags: review?(wmccloskey)
Comment on attachment 8848147 [details] [diff] [review]
Bug1345102.patch

Review of attachment 8848147 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ +39,5 @@
>  #include "mozilla/ipc/Faulty.h"
>  #endif
>  
> +// Use OS specific iovec array limit.
> +static const size_t kMaxIOVecSize = IOV_MAX;

Please #include <limits.h>, which I hope is enough to get a definition for IOV_MAX. There seems to be a lot of confusion about where it's defined.
Attachment #8848147 - Flags: review?(wmccloskey) → review+
Sorry, I'm not sure wheter I should add the include line or someone else during the final commit (since I got review+)? Can you please advice what to do now?
Flags: needinfo?(wmccloskey)
Attached patch Bug1345102.patch (obsolete) — Splinter Review
Attachment #8848147 - Attachment is obsolete: true
Attachment #8854024 - Flags: review?(wmccloskey)
I pushed this patch to our CI infrastructure and it looks like it fails to compile on Android:
https://treeherder.mozilla.org/logviewer.html#?job_id=88482000&repo=try&lineNumber=7091

Maybe you can sprinkle #ifdef ANDROID or something in here? It seems like IOV_MAX just isn't something we can rely on being defined everywhere.
Flags: needinfo?(wmccloskey)
Attached patch Bug1345102.patchSplinter Review
Attachment #8854024 - Attachment is obsolete: true
Attachment #8854024 - Flags: review?(wmccloskey)
Attachment #8854347 - Flags: review?(wmccloskey)
I was about to say that Android is wrong, but then I took a closer look at the standard.  http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html says:

> A definition of one of the symbolic constants in the following list shall be omitted from <limits.h> on specific implementations where the corresponding value is equal to or greater than the stated minimum, but is unspecified.

For IOV_MAX, the stated minimum is _XOPEN_IOV_MAX (== 16).  But the hard-coded 256 was apparently working fine on Android (and B2G), and lowering it might cause performance changes or other problems.

So basically I wrote all of that to say that I agree with billm in comment #7 and #ifdef ANDROID seems like the right fix for this patch.
Comment on attachment 8854347 [details] [diff] [review]
Bug1345102.patch

Review of attachment 8854347 [details] [diff] [review]:
-----------------------------------------------------------------

I revised the patch a bit and I'll push it now.
Attachment #8854347 - Flags: review?(wmccloskey) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20e5a984fc18
ipc_channel_posix.cc should use OS defined IOV_MAX macro (r=billm)
https://hg.mozilla.org/mozilla-central/rev/20e5a984fc18
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.