Bug 189709 – InjectedBundle parameters often need initialization function called before unarchiving

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

In Bug 186788 I revised the InjectedBundle serialization to use NSSecureCoding. Since some users of InjectedBundles want to serialize custom classes, they need to tell the WebContent process which classes it is allowed to safely serialize. This is generally done in the bundle initialization function.

Unfortunately, InjectedBundle::initialize attempts to decode the bundle parameters before running the initialization function, so misses any custom classes needed by the embedding application.

This patch checks if serialization of the bundle parameters succeeded. If it did not, it attempts to decode the parameters after the initialization function runs so that it can take advantage of any new classes registered by the embedding application.

[Radar WebKit Bug Importer]

<rdar://problem/44573653>

[Brent Fulgham]

Created attachment 350048 [details]
Patch

[Brent Fulgham]

Created attachment 350119 [details]
Patch

[Chris Dumez]

Comment on attachment 350119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350119&action=review

Could you clarify why we try to decode before and after the initialization function? Why do not we always decode after?

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundle.h:163
> +    bool decodeBundleParameters(RefPtr<API::Data>);

Why is this public?
Also, why does it need to ref its parameter? can it simply be a API::Data*?

[Brent Fulgham]

Comment on attachment 350119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350119&action=review

>> Source/WebKit/WebProcess/InjectedBundle/InjectedBundle.h:163
>> +    bool decodeBundleParameters(RefPtr<API::Data>);
> 
> Why is this public?
> Also, why does it need to ref its parameter? can it simply be a API::Data*?

Sure -- it could be private and take a pointer. I'll change that.

[Brent Fulgham]

Created attachment 350242 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 350119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350119&action=review
> 
> Could you clarify why we try to decode before and after the initialization
> function? Why do not we always decode after?

You did not answer my question.

[Brent Fulgham]

(In reply to Chris Dumez from comment #7)
> > Could you clarify why we try to decode before and after the initialization
> > function? Why do not we always decode after?
> 
> You did not answer my question.

Gosh! Somehow I missed this!

I considered that, but wasn't sure if there might be cases where a initialization function depends on the contents of the bundle parameters. I can't give an actual instance where this is known to happen, but if there was such a bundle moving the code could break it.

But I'm fine switching it around so we never pass through the 'throw' case.

[Brent Fulgham]

Created attachment 350248 [details]
Patch

[Chris Dumez]

Comment on attachment 350248 [details]
Patch

r=me if bots are happy.

[WebKit Commit Bot]

Comment on attachment 350248 [details]
Patch

Clearing flags on attachment: 350248

Committed r236289: <https://trac.webkit.org/changeset/236289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change caused 8 TestWebKitAPI.ContentFiltering test failures:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/8104

[Ryan Haddad]

Reverted r236289 for reason:

Caused 8 TestWebKitAPI.ContentFiltering test failures.

Committed r236304: <https://trac.webkit.org/changeset/236304>

[Ryan Haddad]

From a debug bot:

        ASSERTION FAILED: testPlugInClassName
        /Volumes/Data/slave/sierra-debug/build/Tools/TestWebKitAPI/cocoa/WebProcessPlugIn/WebProcessPlugIn.mm(44) : -[WebProcessPlugIn webProcessPlugIn:initializeWithObject:]
        1   0x126b6a4f9 WTFCrash
        2   0x11797235b WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x117978e59 -[WebProcessPlugIn webProcessPlugIn:initializeWithObject:]
        4   0x10df40b64 WebKit::InjectedBundle::initialize(WebKit::WebProcessCreationParameters const&, API::Object*)
        5   0x10e1fb078 WebKit::InjectedBundle::create(WebKit::WebProcessCreationParameters&, API::Object*)
        6   0x10e18e826 WebKit::WebProcess::initializeWebProcess(WebKit::WebProcessCreationParameters&&)
        7   0x10e60ead6 void IPC::callMemberFunctionImpl<WebKit::WebProcess, void (WebKit::WebProcess::*)(WebKit::WebProcessCreationParameters&&), std::__1::tuple<WebKit::WebProcessCreationParameters>, 0ul>(WebKit::WebProcess*, void (WebKit::WebProcess::*)(WebKit::WebProcessCreationParameters&&), std::__1::tuple<WebKit::WebProcessCreationParameters>&&, std::__1::integer_sequence<unsigned long, 0ul>)
        8   0x10e60e8e8 void IPC::callMemberFunction<WebKit::WebProcess, void (WebKit::WebProcess::*)(WebKit::WebProcessCreationParameters&&), std::__1::tuple<WebKit::WebProcessCreationParameters>, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WebKit::WebProcessCreationParameters>&&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(WebKit::WebProcessCreationParameters&&))
        9   0x10e608964 void IPC::handleMessage<Messages::WebProcess::InitializeWebProcess, WebKit::WebProcess, void (WebKit::WebProcess::*)(WebKit::WebProcessCreationParameters&&)>(IPC::Decoder&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(WebKit::WebProcessCreationParameters&&))
        10  0x10e606475 WebKit::WebProcess::didReceiveWebProcessMessage(IPC::Connection&, IPC::Decoder&)
        11  0x10e191d8b WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
        12  0x10d49fbfa IPC::Connection::dispatchMessage(IPC::Decoder&)
        13  0x10d492d51 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
        14  0x10d4a0713 IPC::Connection::dispatchOneIncomingMessage()
        15  0x10d4bd758 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
        16  0x10d4bd669 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
        17  0x126b917cd WTF::Function<void ()>::operator()() const
        18  0x126be9873 WTF::RunLoop::performWork()
        19  0x126bea204 WTF::RunLoop::performWork(void*)
        20  0x7fff790a43e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
        21  0x7fff7908565c __CFRunLoopDoSources0
        22  0x7fff79084b46 __CFRunLoopRun
        23  0x7fff79084544 CFRunLoopRunSpecific
        24  0x7fff785e3ebc RunCurrentEventLoopInMode
        25  0x7fff785e3cf1 ReceiveNextEventCommon
        26  0x7fff785e3b26 _BlockUntilNextEventMatchingListInModeWithFilter
        27  0x7fff76b7aa54 _DPSNextEvent
        28  0x7fff772f67ee -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
        29  0x7fff76b6f3db -[NSApplication run]
        30  0x7fff76b39e0e NSApplicationMain
        31  0x7fff8ee9d8c7 _xpc_objc_main

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK1%20%28Tests%29/builds/9628/steps/run-api-tests/logs/stdio

[Brent Fulgham]

Created attachment 367406 [details]
Patch

[Brent Fulgham]

Created attachment 367422 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367422 [details]
Patch

Clearing flags on attachment: 367422

Committed r244299: <https://trac.webkit.org/changeset/244299>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r244299 for reason:

Breaks internal tests.

Committed r244431: <https://trac.webkit.org/changeset/244431>

[Brent Fulgham]

This should be relanded once <rdar://problem/50024284> is finished.

[Brent Fulgham]

Created attachment 367752 [details]
Patch for landing

[Brent Fulgham]

Created attachment 367753 [details]
Patch for landing

[Brent Fulgham]

Created attachment 367754 [details]
Patch for landing

[Brent Fulgham]

(In reply to Brent Fulgham from comment #21)
> This should be relanded once <rdar://problem/50024284> is finished.

Disregard this statement. It was not true for this bug.

[WebKit Commit Bot]

Comment on attachment 367754 [details]
Patch for landing

Clearing flags on attachment: 367754

Committed r244437: <https://trac.webkit.org/changeset/244437>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r244437 for reason:

Still breaks internal tests.

Committed r244508: <https://trac.webkit.org/changeset/244508>

[Brent Fulgham]

Created attachment 396437 [details]
Patch for landing

[EWS]

Committed r260081: <https://trac.webkit.org/changeset/260081>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396437 [details].