<div dir="ltr">Hi Pekka<div><br><div>Thank you for your comment. They are very helpful.</div><div><br></div><div>

<span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>that's a good find. If I understand correctly, we do not handle the</span><br style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>case where a weston_view is effectively unmapped by removing it from a</span><br style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>weston_layer. As weston_compositor::view_list is never torn down</span><br style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>properly but simply rebuilt, the unmapped view is left with dangling</span><br style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>pointers, and those will be hit when finally destroying the view.</span><br style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">

<br></div><div>Your explanation is exactly correct. <br></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">I have verified the optional 1 can't work. </span></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">optional 2</span></div><div><span style="font-size:14px"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>> weston_view_damage_below(</span><wbr style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">shsurf->fullscreen.black_view)</span><wbr style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">;</span><br style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>> +</span><br style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>>  wl_list_remove(&shsurf-></span><wbr style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">fullscreen.black_view->link);</span>

<br></span></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">>

<span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Removing without init might cause issues with a potential double-remove</span><br style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">>on e.g. view destruction.</span>

</span></div><div><span style="font-size:14px"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></span></div><div><span style="font-size:14px">yes. 

<span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">I add the initialization for the list.</span>

</span></div><div><span style="font-size:14px"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></span></div><div><span style="font-size:14px">+ wl_list_init

<span style="font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;color:rgb(80,0,80);background-color:rgb(255,255,255);float:none;display:inline">(&shsurf-></span><wbr style="font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;color:rgb(80,0,80);background-color:rgb(255,255,255)"><span style="font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;color:rgb(80,0,80);background-color:rgb(255,255,255);float:none;display:inline">fullscreen.black_view->link);</span><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><span> </span></span>

</span></div><div><span style="font-size:14px"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><span><br></span></span></span></div><div><span style="font-size:14px">I agree with you weston_view_unmap maybe better for optional 2.  <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Since I find crashes <span> </span></span><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;font-size:14px">caused by invalid pointer, I only concern how to make the view.link to point  the right address.</span>

</span></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">No matter what functions use here, </span><span style="font-size:14px">this only handle the black views.</span></div><div><span style="font-size:14px">Are there other views have the same problems?  Especially in ivi shell, I find the crash exist in many  scenarios. </span></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">How would weston to resolve the potential risks? Thank you.</span></div></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">Best Regards</span></div><div><span style="font-size:14px">Nancy</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">2018-04-03 20:28 GMT+08:00 Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, 29 Mar 2018 11:01:47 +0800<br>
zou lan <<a href="mailto:nancy.lan.zou@gmail.com">nancy.lan.zou@gmail.com</a>> wrote:<br>
<br>
> Hi Pekka & all<br>
><br>
> When I run some full screen applications under desktop shell,  then kill<br>
> some applications randomly, I find<br>
> crashes maybe happened sometimes.<br>
><br>
> Crash call stack:<br>
> weston_view_destroy-->weston_<wbr>view_unmap-->wl_list_reomve(&<wbr>view->link).<br>
> The view is black view.<br>
<br>
</span>Hi,<br>
<br>
that's a good find. If I understand correctly, we do not handle the<br>
case where a weston_view is effectively unmapped by removing it from a<br>
weston_layer. As weston_compositor::view_list is never torn down<br>
properly but simply rebuilt, the unmapped view is left with dangling<br>
pointers, and those will be hit when finally destroying the view.<br>
<br>
Would that be an accurate explanation?<br>
<span class=""><br>
> Then I debug code, find that shell.c  lower_fullscreen_layer function only<br>
> delete the layer of black view from layer_list, but not handle the<br>
> &view.link.<br>
> lower_fullscreen_layer()<br>
> {<br>
> .....<br>
>     /* Hide the black view */<br>
><br>
> weston_layer_entry_remove(&<wbr>shsurf->fullscreen.black_view-<wbr>>layer_link);<br>
><br>
> wl_list_init(&shsurf-><wbr>fullscreen.black_view->layer_<wbr>link.link);<br>
<br>
</span>This wl_list_init() seems to be redundant, btw.<br>
<br>
><br>
> weston_view_damage_below(<wbr>shsurf->fullscreen.black_view)<wbr>;<br>
<br>
Could this path simply call weston_view_unmap() to unmap the black_view?<br>
<br>
It effectively is unmapping the view, but forgetting to clear the<br>
'is_mapped' boolean, which later leads to accessing weston_view::link<br>
dangling pointers. weston_view_unmap() does a lot more which might be<br>
necessary as well.<br>
<span class=""><br>
><br>
> }<br>
><br>
> The code will be called when start second full screen application.<br>
><br>
> when weston build view list again, the first surface's black view is not in<br>
> the view list, the list &view.link maybe point some invalid address.<br>
><br>
> I think ivi shell will have the same problems too.  I have a temporary<br>
> patch to resolve this problem. I want to discuss it in the community.<br>
><br>
> option 1<br>
> --- a/libweston/compositor.c<br>
> +++ b/libweston/compositor.c<br>
> @@ -2195,7 +2195,7 @@ view_list_add(struct weston_compositor *compositor,<br>
>         struct weston_subsurface *sub;<br>
><br>
>         weston_view_update_transform(<wbr>view);<br>
> -<br>
> +       wl_list_remove(&view.link);<br>
<br>
</span>For better or worse, the weston_view::link is deliberately left with<br>
garbage pointers when the weston_compositor::view_list is discarded for<br>
a rebuild.<br>
<br>
Wouldn't this addition cause weston_compositor::view_list to become<br>
corrupted if the link removed here happened to be the first or the last<br>
in the old list order? Or maybe be get lucky most times, and it would<br>
need quite special circumstances to trigger the corruption.<br>
<br>
I still think this approach is not good.<br>
<span class=""><br>
><br>
> option 2:<br>
> --- a/desktop-shell/shell.c<br>
> +++ b/desktop-shell/shell.c<br>
> @@ -3651,6 +3651,7 @@ lower_fullscreen_layer(struct desktop_shell *shell,<br>
><br>
> weston_layer_entry_remove(&<wbr>shsurf->fullscreen.black_view-<wbr>>layer_link);<br>
><br>
> wl_list_init(&shsurf-><wbr>fullscreen.black_view->layer_<wbr>link.link);<br>
><br>
> weston_view_damage_below(<wbr>shsurf->fullscreen.black_view)<wbr>;<br>
> +<br>
>  wl_list_remove(&shsurf-><wbr>fullscreen.black_view->link);<br>
<br>
</span>Removing without init might cause issues with a potential double-remove<br>
on e.g. view destruction.<br>
<span class=""><br>
><br>
>                 }<br>
><br>
> Do you think these options reasonable? Thank you.<br>
<br>
</span>I think using weston_view_unmap() would be the best as I mentioned<br>
above, unless there is some specific reason why it cannot be used. None<br>
come to my mind this time.<br>
<br>
<br>
Thanks,<br>
pq<br>
</blockquote></div><br></div>