Bug 189709 - InjectedBundle parameters often need initialization function called before unarchiving
Summary: InjectedBundle parameters often need initialization function called before un...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 186788
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-18 13:43 PDT by Brent Fulgham
Modified: 2019-04-22 12:51 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.78 KB, patch)
2018-09-18 13:58 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2018-09-19 08:35 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.11 KB, patch)
2018-09-20 11:52 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (6.84 KB, patch)
2018-09-20 12:23 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (10.00 KB, patch)
2019-04-14 14:50 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (9.99 KB, patch)
2019-04-15 10:32 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (10.97 KB, patch)
2019-04-18 14:06 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (10.97 KB, patch)
2019-04-18 14:07 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (10.96 KB, patch)
2019-04-18 14:08 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2018-09-18 13:43:16 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-09-18 13:45:36 PDT
<rdar://problem/44573653>
Comment 2 Brent Fulgham 2018-09-18 13:58:08 PDT
Created attachment 350048 [details]
Patch
Comment 3 Brent Fulgham 2018-09-19 08:35:08 PDT
Created attachment 350119 [details]
Patch
Comment 4 Chris Dumez 2018-09-20 11:05:59 PDT
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*?
Comment 5 Brent Fulgham 2018-09-20 11:37:54 PDT
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.
Comment 6 Brent Fulgham 2018-09-20 11:52:18 PDT
Created attachment 350242 [details]
Patch
Comment 7 Chris Dumez 2018-09-20 12:00:15 PDT
(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.
Comment 8 Brent Fulgham 2018-09-20 12:17:23 PDT
(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.
Comment 9 Brent Fulgham 2018-09-20 12:23:23 PDT
Created attachment 350248 [details]
Patch
Comment 10 Chris Dumez 2018-09-20 12:29:39 PDT
Comment on attachment 350248 [details]
Patch

r=me if bots are happy.
Comment 11 WebKit Commit Bot 2018-09-20 16:11:26 PDT
Comment on attachment 350248 [details]
Patch

Clearing flags on attachment: 350248

Committed r236289: <https://trac.webkit.org/changeset/236289>
Comment 12 WebKit Commit Bot 2018-09-20 16:11:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Ryan Haddad 2018-09-20 20:42:40 PDT
This change caused 8 TestWebKitAPI.ContentFiltering test failures:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/8104
Comment 14 Ryan Haddad 2018-09-20 20:47:56 PDT
Reverted r236289 for reason:

Caused 8 TestWebKitAPI.ContentFiltering test failures.

Committed r236304: <https://trac.webkit.org/changeset/236304>
Comment 15 Ryan Haddad 2018-09-20 20:49:44 PDT
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
Comment 16 Brent Fulgham 2019-04-14 14:50:11 PDT
Created attachment 367406 [details]
Patch
Comment 17 Brent Fulgham 2019-04-15 10:32:44 PDT
Created attachment 367422 [details]
Patch
Comment 18 WebKit Commit Bot 2019-04-15 15:22:05 PDT
Comment on attachment 367422 [details]
Patch

Clearing flags on attachment: 367422

Committed r244299: <https://trac.webkit.org/changeset/244299>
Comment 19 WebKit Commit Bot 2019-04-15 15:22:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Ryan Haddad 2019-04-18 12:16:55 PDT
Reverted r244299 for reason:

Breaks internal tests.

Committed r244431: <https://trac.webkit.org/changeset/244431>
Comment 21 Brent Fulgham 2019-04-18 12:31:08 PDT
This should be relanded once <rdar://problem/50024284> is finished.
Comment 22 Brent Fulgham 2019-04-18 14:06:29 PDT
Created attachment 367752 [details]
Patch for landing
Comment 23 Brent Fulgham 2019-04-18 14:07:15 PDT
Created attachment 367753 [details]
Patch for landing
Comment 24 Brent Fulgham 2019-04-18 14:08:41 PDT
Created attachment 367754 [details]
Patch for landing
Comment 25 Brent Fulgham 2019-04-18 14:22:08 PDT
(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.
Comment 26 WebKit Commit Bot 2019-04-18 14:49:26 PDT
Comment on attachment 367754 [details]
Patch for landing

Clearing flags on attachment: 367754

Committed r244437: <https://trac.webkit.org/changeset/244437>
Comment 27 WebKit Commit Bot 2019-04-18 14:49:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Ryan Haddad 2019-04-22 12:51:44 PDT
Reverted r244437 for reason:

Still breaks internal tests.

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