Closed Bug 1477129 Opened Last year Closed Last year

Use UpdateSharedData rather than SetXPCOMProcessAttributes to send initial shared map data

Categories

(Core :: IPC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(3 files)

We currently use two separate methods to send sharedData maps to the content process: SetXPCOMProcessAttributes for the initial data, and UpdateSharedData for further updates.

This causes two problems:

1) There is apparently a bug on FreeBSD which causes it to mishandle large messages which include file descriptors, and,

2) It makes it easy for the two methods to get out of sync. In particular, the SetXPCOMProcessAttributes method already fails to include BlobImpls for the initial data set.
e10s seems to work again on FreeBSD/amd64 11.2 with these patches.
Comment on attachment 8993557 [details]
Bug 1477129: Part 3 - Re-enable e10s on FreeBSD.

I confirm. With this patch series e10s works fine on FreeBSD.
Attachment #8993557 - Flags: feedback+
Err, I've tested 12.0-CURRENT r336538 amd64 kernel.
Comment on attachment 8993557 [details]
Bug 1477129: Part 3 - Re-enable e10s on FreeBSD.

https://reviewboard.mozilla.org/r/258254/#review265432

Thanks Jan for checking this patch out!
Attachment #8993557 - Flags: review?(nfroyd) → review+
Comment on attachment 8993556 [details]
Bug 1477129: Part 2 - Fix SharedMap blob handling, and add tests.

https://reviewboard.mozilla.org/r/258252/#review265490

I think this is good, I just want to get expanded tests for the second content process to feel a little more solid.

::: commit-message-45ad5:1
(Diff revision 1)
> +Bug 1477129: Part 2 - Fix SharedMap blob handling, and add tests. r?erahm

Can you add details to the commit message that explain what was broken.

::: dom/ipc/ContentParent.cpp:2347
(Diff revision 1)
> +  Unused << SendSetXPCOMProcessAttributes(xpcomInit, initialData, lnfCache,
> +                                          fontList);
> +
>    ipc::WritableSharedMap* sharedData = nsFrameMessageManager::sParentProcessManager->SharedData();
>    sharedData->Flush();
> +  sharedData->SendChanges(this);

Okay, so splitting these two calls reduces the message size I guess? Which makes BSD less sad?

::: dom/ipc/SharedMap.h:354
(Diff revision 1)
>    // child SharedMap instances.
>    void Flush();
>  
>  
> +  // Sends the current set of shared map data to the given content process.
> +  void SendChanges(ContentParent* aContentParent);

This can be `const` right?

::: dom/ipc/SharedMap.cpp
(Diff revision 1)
>      entry->Code(header);
>  
>      offset += entry->Size();
> -
> -    if (entry->BlobCount()) {
> -      mBlobImpls.AppendElements(entry->Blobs());

I suppose this is the crux of the issue.

::: dom/ipc/SharedMap.cpp:381
(Diff revision 1)
>  
>    return Ok();
>  }
>  
>  void
> +WritableSharedMap::SendChanges(ContentParent* aParent)

nit: This always sends even if there aren't changes, maybe just `SendUpdate` or `Send` would make sense.

::: dom/ipc/tests/test_sharedMap.js:217
(Diff revision 1)
> +
> +  equal(await contentPage.spawn("blob0", readBlob), text[0], "Expected text for blob0 in child 1 cpmm");
> +  equal(await contentPage.spawn("blob1", readBlob), text[1], "Expected text for blob1 in child 1 cpmm");
> +
> +  // Start a second child process
> +  Services.prefs.setIntPref(PROCESS_COUNT_PREF, 2);

We should also test structured clones in new processes.
Attachment #8993556 - Flags: review?(erahm) → review-
Comment on attachment 8993556 [details]
Bug 1477129: Part 2 - Fix SharedMap blob handling, and add tests.

https://reviewboard.mozilla.org/r/258252/#review265490

> Okay, so splitting these two calls reduces the message size I guess? Which makes BSD less sad?

It does, but that's not quite the issue. The issue is that FreeBSD can't handle large messages which also contain file descriptors. The problem message is SetXPCOMProcessAttributes, which contains tens of KB of font data, and therefore needs to be split into multiple messages. That works fine when it doesn't contain a file descriptor, but not when it does.

The UpdateSharedData message is never big enough to require splitting.

> This can be `const` right?

Oh, yeah.

> I suppose this is the crux of the issue.

Yeah. Well, one of two cruxes. The other was that the move constructor for StructuredCloneData threw away its blobs.

Apparently the code that I thought used Blobs actually used blob: URLs, so my expectation that would exercise this code turned out to be wrong.

> We should also test structured clones in new processes.

Not sure what you mean... that's what this does.
Comment on attachment 8993555 [details]
Bug 1477129: Part 1 - Handle BlobImpls when move constructing StructuredCloneData.

https://reviewboard.mozilla.org/r/258250/#review265646
Attachment #8993555 - Flags: review?(amarchesini) → review+
Comment on attachment 8993556 [details]
Bug 1477129: Part 2 - Fix SharedMap blob handling, and add tests.

https://reviewboard.mozilla.org/r/258252/#review265806

Thanks for the update, looks good.
Attachment #8993556 - Flags: review?(erahm) → review+
Fwiw i've tested a selfbuilt nightly with those commits, and e10s still works on OpenBSD.
You need to log in before you can comment on or make changes to this bug.