Bug 96197 – [EFL][WK2] Provide implementation for PageClientImpl::processDidCrash()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jinwoo Song]

WebKit2 EFL does not have a implementation for PageClientImpl::processDidCrash(). 
So when WebProcess is crashed, UI process does nothing and left alone as it is.

[Jinwoo Song]

Created attachment 163083 [details]
patch

[Chris Dumez]

Comment on attachment 163083 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1590
> + * Web process was crashed.

"has" crashed

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1592
> + * Emits signal: "process,crashed" with pointer to handle crash.

with pointer to "crash handling boolean" ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:59
> + * - "process,crashed", bool*: reports that web process is crashed.

"has" crashed.

[Jinwoo Song]

Created attachment 163243 [details]
patch

[Jinwoo Song]

(In reply to comment #2)
> (From update of attachment 163083 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163083&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1590
> > + * Web process was crashed.
> 
> "has" crashed
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1592
> > + * Emits signal: "process,crashed" with pointer to handle crash.
> 
> with pointer to "crash handling boolean" ?
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:59
> > + * - "process,crashed", bool*: reports that web process is crashed.
> 
> "has" crashed.
fixed.

[Chris Dumez]

Comment on attachment 163243 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1605
> +        exit(0);

Frankly, I'm a bit scared by this. Why would we exit the app? It seems no other port is doing this.
I think we should simply display an error page.

To my knowledge, the fact that the WebProcess crashed is not a dead end. If you open a new URL, a new WebProcess would be created.

[Gyuyoung Kim]

Comment on attachment 163243 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1599
> +    fprintf(stderr, "WARNING: The web process experienced a crash on '%s'.\n", priv->pageProxy->urlAtProcessExit().utf8().data());

IMO, EINA_LOG_DOM_WARN is better than fprintf for ewk layer.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:59
> + * - "process,crashed", bool*: reports that web process has crashed.

Eina_Bool is correct.

[Kenneth Rohde Christiansen]

Comment on attachment 163243 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:110
>  void PageClientImpl::processDidCrash()
>  {
> -    notImplemented();
> +    ewk_view_process_crashed(m_viewWidget);
>  }

This seems very simplistic. For Qt we handle lots of things relating to our view, history etc. I am not entirely sure where that is done, but I think it would make sense to have a look and analyse it.

As the class name shows this is information meant for the view (ie. client of the page)

[Jinwoo Song]

Comment on attachment 163243 [details]
patch

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

>> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:110
>>  }
> 
> This seems very simplistic. For Qt we handle lots of things relating to our view, history etc. I am not entirely sure where that is done, but I think it would make sense to have a look and analyse it.
> 
> As the class name shows this is information meant for the view (ie. client of the page)

Yes, I also think it's very simple. I look into the Qt and found that Qt handles warning message, resetGestureRecognizers, and load progress. (void QQuickWebViewPrivate::processDidCrash()). But I could not find other place where other things are handled.
As you know, GTK has no implementation for processDidCrash() and default behavior is done in WebPageProxy::processDidCrash().

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1599
>> +    fprintf(stderr, "WARNING: The web process experienced a crash on '%s'.\n", priv->pageProxy->urlAtProcessExit().utf8().data());
> 
> IMO, EINA_LOG_DOM_WARN is better than fprintf for ewk layer.

Okay, I'll use EINA_LOG_DOM_WARN.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1605
>> +        exit(0);
> 
> Frankly, I'm a bit scared by this. Why would we exit the app? It seems no other port is doing this.
> I think we should simply display an error page.
> 
> To my knowledge, the fact that the WebProcess crashed is not a dead end. If you open a new URL, a new WebProcess would be created.

I agree with you and I'll change to only display the error message.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:59
>> + * - "process,crashed", bool*: reports that web process has crashed.
> 
> Eina_Bool is correct.

I'll fix it.

[Jinwoo Song]

Created attachment 163300 [details]
patch.

[Gyuyoung Kim]

Why not request review again ?

[Jinwoo Song]

(In reply to comment #10)
> Why not request review again ?
Okay, I set the flag.

[Gyuyoung Kim]

Comment on attachment 163300 [details]
patch.

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

Looks good to me except for my comment.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:59
> + * - "process,crashed", Eina_Bool*: reports that web process has crashed.

If new signal depends on a parameter which gets a result from callback function, we added comment for parameter as below,

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L71

[Jinwoo Song]

Created attachment 163318 [details]
patch

[Jinwoo Song]

(In reply to comment #12)
> (From update of attachment 163300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163300&action=review
> 
> Looks good to me except for my comment.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:59
> > + * - "process,crashed", Eina_Bool*: reports that web process has crashed.
> 
> If new signal depends on a parameter which gets a result from callback function, we added comment for parameter as below,
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L71

Done. :)

[Gyuyoung Kim]

(In reply to comment #8)

> Yes, I also think it's very simple. I look into the Qt and found that Qt handles warning message, resetGestureRecognizers, and load progress. (void QQuickWebViewPrivate::processDidCrash()). But I could not find other place where other things are handled.
> As you know, GTK has no implementation for processDidCrash() and default behavior is done in WebPageProxy::processDidCrash().

One more thing. I'm not sure if we can reset gesture recognizer like qt port. But, it looks we need to check whether page loading is ongoing or not. IMO, we're able to do similar stuff as qt port for finishing page loading using below functions.

- ewk_view_load_progress_get
- ewk_view_load_progress_changed

[Jinwoo Song]

(In reply to comment #15)
> (In reply to comment #8)
> 
> > Yes, I also think it's very simple. I look into the Qt and found that Qt handles warning message, resetGestureRecognizers, and load progress. (void QQuickWebViewPrivate::processDidCrash()). But I could not find other place where other things are handled.
> > As you know, GTK has no implementation for processDidCrash() and default behavior is done in WebPageProxy::processDidCrash().
> 
> One more thing. I'm not sure if we can reset gesture recognizer like qt port. But, it looks we need to check whether page loading is ongoing or not. IMO, we're able to do similar stuff as qt port for finishing page loading using below functions.
> 
> - ewk_view_load_progress_get
> - ewk_view_load_progress_changed

Okay, I'll do this and upload a patch again. Thanks for advice. :-)

[Jinwoo Song]

Created attachment 163769 [details]
patch

[Gyuyoung Kim]

Comment on attachment 163769 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_private.h:34
> +extern int _ewk_log_dom;

Nit : A new line is needed.

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:104
> +void ewk_view_process_crashed(Evas_Object* ewkView);

ditto.

[Jinwoo Song]

Created attachment 163771 [details]
patch.

[Jinwoo Song]

(In reply to comment #18)
> (From update of attachment 163769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163769&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_private.h:34
> > +extern int _ewk_log_dom;
> 
> Nit : A new line is needed.
Done.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:104
> > +void ewk_view_process_crashed(Evas_Object* ewkView);
> 
> ditto.
Done.

[Simon Hausmann]

Comment on attachment 163771 [details]
patch.

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

> Source/WebKit2/UIProcess/API/efl/ewk_private.h:34
> +extern int _ewk_log_dom;

Just a thought, wouldn't it be simpler to do the import in ewk_view.cpp instead of here? :)

[Chris Dumez]

Comment on attachment 163771 [details]
patch.

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

> Source/WebKit2/ChangeLog:13
> +        with a url at process exit. And after checking if loading was onging, if so, set

"ongoing"

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1644
> +

We could even show an HTML error page here, like Qt port is doing. (e.g. using ewk_view_html_string_load())

> Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:104
> +void ewk_view_process_crashed(Evas_Object* ewkView);

wouldn't it be clearer if we used ewk_view_webprocess_crashed()?

[Jinwoo Song]

(In reply to comment #21)
> (From update of attachment 163771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163771&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_private.h:34
> > +extern int _ewk_log_dom;
> 
> Just a thought, wouldn't it be simpler to do the import in ewk_view.cpp instead of here? :)
Actually, I referenced WebKit1 EFL in ewk_private.h.(Source/WebKit/efl/ewk/ewk_private.h)
Also, I'm considering to add Eina Log macros in the ewk_private.h in WebKit2 so as to use the macros in other use cases. That's the reason I put it there. :)

[Jinwoo Song]

(In reply to comment #22)
> (From update of attachment 163771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163771&action=review
> 
> > Source/WebKit2/ChangeLog:13
> > +        with a url at process exit. And after checking if loading was onging, if so, set
> 
> "ongoing"
Fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1644
> > +
> 
> We could even show an HTML error page here, like Qt port is doing. (e.g. using ewk_view_html_string_load())
I applied your suggestion. 
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:104
> > +void ewk_view_process_crashed(Evas_Object* ewkView);
> 
> wouldn't it be clearer if we used ewk_view_webprocess_crashed()?
Yes, I agree with you. I changed the name of the function and callback.

[Jinwoo Song]

Created attachment 163798 [details]
patch applied Christophe's comments.

[Gyuyoung Kim]

Comment on attachment 163798 [details]
patch applied Christophe's comments.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1655
> +   

Unneeded line.

[Jinwoo Song]

Created attachment 163802 [details]
patch

[Jinwoo Song]

(In reply to comment #26)
> (From update of attachment 163798 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163798&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1655
> > +   
> 
> Unneeded line.
Fixed.

[Chris Dumez]

Comment on attachment 163802 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1648
> +        ewk_view_html_string_load(ewkView, errorHTML.utf8().data(), 0, 0);

The last argument is the unreachable URL. You should probably pass url.utf8().data().

[Gyuyoung Kim]

Comment on attachment 163802 [details]
patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1648
>> +        ewk_view_html_string_load(ewkView, errorHTML.utf8().data(), 0, 0);
> 
> The last argument is the unreachable URL. You should probably pass url.utf8().data().

How about pass "file:///" ?

[Chris Dumez]

Comment on attachment 163802 [details]
patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1648
>>> +        ewk_view_html_string_load(ewkView, errorHTML.utf8().data(), 0, 0);
>> 
>> The last argument is the unreachable URL. You should probably pass url.utf8().data().
> 
> How about pass "file:///" ?

I don't understand your comment. How is "file:///" the unreachable url? The unreachable url is priv->pageProxy->urlAtProcessExit().

[Kenneth Rohde Christiansen]

Comment on attachment 163802 [details]
patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1648
>>> +        ewk_view_html_string_load(ewkView, errorHTML.utf8().data(), 0, 0);
>> 
>> The last argument is the unreachable URL. You should probably pass url.utf8().data().
> 
> How about pass "file:///" ?

Now can you make sure the process is alive here?

Also webkit fills in <html> and <body> for you

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1653
> +        // Check if loading was ongoing, when process crashed.
> +        double loadProgress = ewk_view_load_progress_get(ewkView);
> +        if (loadProgress >= 0 && loadProgress < 1)
> +            ewk_view_load_progress_changed(ewkView, 1);

This seems like something that should be handled even if the handled is true, I would probably move it to ::processDidCrash()

[Jinwoo Song]

Created attachment 163825 [details]
patch

Applied the comments by Kenneth and Christophe.

[Gyuyoung Kim]

Comment on attachment 163825 [details]
patch

Attachment 163825 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13833737

[Gyuyoung Kim]

Comment on attachment 163825 [details]
patch

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

> don't understand your comment. How is "file:///" the unreachable url? The unreachable url is priv->pageProxy->urlAtProcessExit().

There was misunderstanding. The unreachable url is right. Thanks.

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:110
> +    double loadProgress = ewk_view_load_progress_get(m_viewWidget);

Need to include ewk_view.h

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1646
> +        // Display a process crash page

process -> webprocess ?

[Kenneth Rohde Christiansen]

> process -> webprocess ?

-> web process

[Jinwoo Song]

Created attachment 164008 [details]
patch to fix build error.

[Jinwoo Song]

(In reply to comment #35)
> (From update of attachment 163825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163825&action=review
> 
> > don't understand your comment. How is "file:///" the unreachable url? The unreachable url is priv->pageProxy->urlAtProcessExit().
> 
> There was misunderstanding. The unreachable url is right. Thanks.
> 
> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:110
> > +    double loadProgress = ewk_view_load_progress_get(m_viewWidget);
> 
> Need to include ewk_view.h
Added missing header. silly mistake :(
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1646
> > +        // Display a process crash page
> 
> process -> webprocess ?

[Jinwoo Song]

(In reply to comment #36)
> > process -> webprocess ?
> 
> -> web process
Fixed.

[Gyuyoung Kim]

Comment on attachment 164008 [details]
patch to fix build error.

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

> Source/WebKit2/ChangeLog:14
> +        with a url at process exit and load a error page.

process -> web process ?

[Jinwoo Song]

Created attachment 164021 [details]
patch for landing.

[Jinwoo Song]

(In reply to comment #40)
> (From update of attachment 164008 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164008&action=review
> 
> > Source/WebKit2/ChangeLog:14
> > +        with a url at process exit and load a error page.
> 
> process -> web process ?
Fixed.

[Gyuyoung Kim]

Comment on attachment 164021 [details]
patch for landing.

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1646
> +        // Display a process crash page

process -> web process.

[Jinwoo Song]

Created attachment 164276 [details]
patch for landing.

[Jinwoo Song]

(In reply to comment #43)
> (From update of attachment 164021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164021&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1646
> > +        // Display a process crash page
> 
> process -> web process.
modified the comment to 
// Display an error page

[Gyuyoung Kim]

Comment on attachment 164276 [details]
patch for landing.

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

I already set r+. So, if you add my name to reviewer field in ChangeLog, you don't need to request r+ again. Just request cq? again.

> Source/WebKit2/ChangeLog:13
> +        If the application does not handle the crash event, show a Eina Log warning message

s/a Eina/an Eina/s

> Source/WebKit2/ChangeLog:14
> +        with a url at web process exit and load a error page.

s/a error/an error/s

[Jinwoo Song]

(In reply to comment #46)
> (From update of attachment 164276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164276&action=review
> 
> I already set r+. So, if you add my name to reviewer field in ChangeLog, you don't need to request r+ again. Just request cq? again.
Okay, I see. Thanks for the tip. :)
> 
> > Source/WebKit2/ChangeLog:13
> > +        If the application does not handle the crash event, show a Eina Log warning message
> 
> s/a Eina/an Eina/s
I'll fix it.
> 
> > Source/WebKit2/ChangeLog:14
> > +        with a url at web process exit and load a error page.
> 
> s/a error/an error/s
I'll fix it.

[Jinwoo Song]

Created attachment 164284 [details]
patch for landing.

[WebKit Review Bot]

Comment on attachment 164284 [details]
patch for landing.

Clearing flags on attachment: 164284

Committed r128690: <http://trac.webkit.org/changeset/128690>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.