| Summary: | [EFL][WK2] Provide implementation for PageClientImpl::processDidCrash() | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jinwoo Song <jinwoo7.song> | ||||||||||||||||||||||||||||
| Component: | WebKit EFL | Assignee: | Jinwoo Song <jinwoo7.song> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | cdumez, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Jinwoo Song
2012-09-09 00:21:26 PDT
Created attachment 163083 [details]
patch
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. Created attachment 163243 [details]
patch
(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. 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. 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. 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) 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. Created attachment 163300 [details]
patch.
Why not request review again ? (In reply to comment #10) > Why not request review again ? Okay, I set the flag. 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 Created attachment 163318 [details]
patch
(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. :) (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 (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. :-) Created attachment 163769 [details]
patch
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. Created attachment 163771 [details]
patch.
(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. 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? :) 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()? (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. :) (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. Created attachment 163798 [details]
patch applied Christophe's comments.
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. Created attachment 163802 [details]
patch
(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. 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(). 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:///" ? 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(). 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() Created attachment 163825 [details]
patch
Applied the comments by Kenneth and Christophe.
Comment on attachment 163825 [details] patch Attachment 163825 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13833737 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 ? > process -> webprocess ?
-> web process
Created attachment 164008 [details]
patch to fix build error.
(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 ? (In reply to comment #36) > > process -> webprocess ? > > -> web process Fixed. 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 ? Created attachment 164021 [details]
patch for landing.
(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. 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. Created attachment 164276 [details]
patch for landing.
(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 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 (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. Created attachment 164284 [details]
patch for landing.
Comment on attachment 164284 [details] patch for landing. Clearing flags on attachment: 164284 Committed r128690: <http://trac.webkit.org/changeset/128690> All reviewed patches have been landed. Closing bug. |