After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 610279 - "st_widget_get_theme_node called on a widget not in a stage"
"st_widget_get_theme_node called on a widget not in a stage"
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 682383 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-02-17 16:59 UTC by Dan Winship
Modified: 2013-07-03 16:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
st_widget_get_theme_node: break the widget's CSS style if called too early (3.12 KB, patch)
2010-02-17 16:59 UTC, Dan Winship
reviewed Details | Review
st_widget_get_theme_node: g_error if called on an unparented widget (1.18 KB, patch)
2010-02-17 20:26 UTC, Dan Winship
committed Details | Review
Fix invalid call to st_widget_get_theme_node() in WindowOverlay (1.89 KB, patch)
2010-02-18 11:53 UTC, Florian Müllner
reviewed Details | Review
Fix invalid call to st_widget_get_theme_node() in WindowOverlay (2.41 KB, patch)
2010-02-22 12:21 UTC, Florian Müllner
committed Details | Review
st: Be more forgiving when calling get_theme_node() on unstaged widgets (1.40 KB, patch)
2013-06-24 18:54 UTC, Florian Müllner
committed Details | Review

Description Dan Winship 2010-02-17 16:59:20 UTC
Three times in the last week or so I've had to point out where people's patches were causing this warning. (bug 607244 comment 15, bug 609777 comment 14, bug 610049 comment 9). We need to make this more noticeable.

One possibility would be to have st_widget_get_theme_node() crash or (probably equivalently) return NULL in this case. Another possibility is to just return nonsensical CSS in response to their nonsensical request.
Comment 1 Dan Winship 2010-02-17 16:59:37 UTC
Created attachment 154055 [details] [review]
st_widget_get_theme_node: break the widget's CSS style if called too early

It's wrong to do anything that requires looking up a widget's style
before you add the widget to the stage, since its final style may
depend on properties inherited from its parents.
st_widget_get_theme_node() emits a warning in this case, but many
would-be contributors apparently don't notice. Help them out.
Comment 2 Colin Walters 2010-02-17 17:05:50 UTC
Review of attachment 154055 [details] [review]:

I'd rather turn it into g_error.  Even better have some sort of test suite that catches this kind of stuff but that's a harder patch of course =)
Comment 3 Owen Taylor 2010-02-17 20:24:12 UTC
I don't see why this is any more of a problem than any other g_warning g_critical that is being spewed by patches and not caught. If that's a problem, we need to either:

 A) Make them fatal
 B) Take error messages off of the controlling terminal and do something like log to a file and make the bad ones blink on the screen

And in most of the uses of st_widget_get_theme_node() that we are doing, I don't think your approach helps any - presumably we are looking up some custom property, so it won't be in the "obviously broken" CSS style.

I don't think returning a GError here makes sense - GError is only supposed to be used when it's something that the program can reasonably handle and do something about - usually in cases where an error should be displayed to the user. It's not supposed to be used for cases that represent buggy programs. Because there is no way that a program can reasonably handle its own bugs.

It would be nice if we had some way of throwing informative exceptions from C code back into JS, but we don't. It's either g_warning, g_critical(), or g_error().
Comment 4 Dan Winship 2010-02-17 20:26:24 UTC
Created attachment 154079 [details] [review]
st_widget_get_theme_node: g_error if called on an unparented widget

the g_error version

I wasn't sure if this was too harsh... in particular, if a patch introduced
such a bug, but only in a slightly-obscure case (say, dragging a window from
one workspace to another in the overview), such a bug might make it through
patch review and into git.
Comment 5 Dan Winship 2010-02-17 20:35:30 UTC
(In reply to comment #3)
>  B) Take error messages off of the controlling terminal and do something like
> log to a file and make the bad ones blink on the screen

that was my other thought (the "blink on the screen" bit). maybe we could use the message tray :-)

(well, unless the bug is in the message tray code...)

> And in most of the uses of st_widget_get_theme_node() that we are doing, I
> don't think your approach helps any - presumably we are looking up some custom
> property, so it won't be in the "obviously broken" CSS style.

right, but once you hit that code path, the widget gets permanently stuck to the broken CSS, so once you do put it on the stage somewhere, it will have a magenta background and a cyan border, etc.

> I don't think returning a GError here makes sense

agreed, i was assuming walters meant g_error() as opposed to g_warning()
Comment 6 Dan Winship 2010-02-17 20:38:44 UTC
(oh, also, most of the cases of this are people trying to get the height or width of an actor before putting it on the stage, and the st_widget_get_theme_node() call happens inside the guts of st.)
Comment 7 Florian Müllner 2010-02-18 11:53:24 UTC
Created attachment 154121 [details] [review]
Fix invalid call to st_widget_get_theme_node() in WindowOverlay

(In reply to comment #4)
> I wasn't sure if this was too harsh... in particular, if a patch introduced
> such a bug, but only in a slightly-obscure case (say, dragging a window from
> one workspace to another in the overview), such a bug might make it through
> patch review and into git.

Of couse this absurd example case has been chosen arbitrarily ...
Comment 8 drago01 2010-02-18 15:42:49 UTC
(In reply to comment #0)
> Three times in the last week or so I've had to point out where people's patches
> were causing this warning. (bug 607244 comment 15, bug 609777 comment 14, bug
> 610049 comment 9). We need to make this more noticeable.
> 
> One possibility would be to have st_widget_get_theme_node() crash or (probably
> equivalently) return NULL in this case. Another possibility is to just return
> nonsensical CSS in response to their nonsensical request.

Well the issue is that I tend to not notice warnings unless something does not work as expected. (I stopped keeping a terminal window open while running g-s a while ago because closing it by accident has an ugly side effect).
I.e if something is wrong I go look for warnings to see what might show up there, but in those cases it did what I expected it to do so the warnings went unnoticed.
(I know now that doing it that way is wrong but it wasn't obvious that it is)
Comment 9 Owen Taylor 2010-02-19 19:49:41 UTC
Review of attachment 154121 [details] [review]:

::: js/ui/workspace.js
@@ +398,3 @@
         parentActor.add_actor(this.closeButton);
+
+        button.hide();

What does this have to do with the patch?

@@ +536,3 @@
     _onStyleChanged: function() {
+        if (!this.title.get_parent() || !this.closeButton.get_parent())
+            return;

Hmm, a bit ugly, but OK. Needs a comment though above this along the lines of 

 // Wait to run until both have been added to the actor hierarchy
Comment 10 Florian Müllner 2010-02-19 20:22:40 UTC
(In reply to comment #9)
> Review of attachment 154121 [details] [review]:
> 
> ::: js/ui/workspace.js
> @@ +398,3 @@
>          parentActor.add_actor(this.closeButton);
> +
> +        button.hide();
> 
> What does this have to do with the patch?

The widget style is not updated until the widget is mapped - so without that change the first invocation of _onStyleChanged() returns immediately because the button has not been parented yet and the second invocation does not occur until the button is shown (read: the pointer hovers over the window clone) ...

Yeah, I'd suggest another comment ...


> @@ +536,3 @@
>      _onStyleChanged: function() {
> +        if (!this.title.get_parent() || !this.closeButton.get_parent())
> +            return;
> 
> Hmm, a bit ugly, but OK. Needs a comment though above this along the lines of 
> 
>  // Wait to run until both have been added to the actor hierarchy


What about something along the lines of:

  let _relayout = false;
  if (this.title.get_parent()) {
     _relayout = true;
     ...
  }
  if (this.closeButton.get_parent()) {
      _relayout = true;
      ...
  }
  if (_relayout)
    this.parentActor.queue_relayout();

Not sure if that's less ugly ... maybe the proper thing to do would be to add a container widget to WindowOverlay and add both widgets there?
Comment 11 Owen Taylor 2010-02-19 20:30:52 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > Review of attachment 154121 [details] [review] [details]:
> > 
> > ::: js/ui/workspace.js
> > @@ +398,3 @@
> >          parentActor.add_actor(this.closeButton);
> > +
> > +        button.hide();
> > 
> > What does this have to do with the patch?
> 
> The widget style is not updated until the widget is mapped - so without that
> change the first invocation of _onStyleChanged() returns immediately because
> the button has not been parented yet and the second invocation does not occur
> until the button is shown (read: the pointer hovers over the window clone) ...
> 
> Yeah, I'd suggest another comment ...

OK. then the easy fix here is something I was going to suggest, but decided wasn't prettier than the way you did it.

Move the style-changed connections to the end of the function and do:

 title.connect('style-changed', ...)
 button.connect('style-changed', ...)
 self._onStyleChanged()
Comment 12 Florian Müllner 2010-02-22 12:21:31 UTC
Created attachment 154382 [details] [review]
Fix invalid call to st_widget_get_theme_node() in WindowOverlay

(In reply to comment #11)
> Move the style-changed connections to the end of the function and do:
> 
>  title.connect('style-changed', ...)
>  button.connect('style-changed', ...)
>  self._onStyleChanged()

Mmmmh, still a little ugly (the call to _onStyleChanged has to be done conditionally), but nevertheless nicer than the original patch ...
Comment 13 Dan Winship 2010-02-22 15:43:21 UTC
> Mmmmh, still a little ugly

if it helps, note that queue_relayout() is idempotent until the relayout actually happens, so you don't need to be careful about only calling it once as you suggested in comment #10. So you could just split _onStyleChanged into separate _onTitleStyleChanged and _onCloseButtonStyleChanged methods, and call queue_relayout() from both of them; if the styles change at the same time, it will still only cause a single relayout.
Comment 14 Florian Müllner 2010-02-22 16:44:46 UTC
(In reply to comment #13)
> if it helps, note that queue_relayout() is idempotent until the relayout
> actually happens, so you don't need to be careful about only calling it once as
> you suggested in comment #10. So you could just split _onStyleChanged into
> separate _onTitleStyleChanged and _onCloseButtonStyleChanged methods, and call
> queue_relayout() from both of them; if the styles change at the same time, it
> will still only cause a single relayout.

I don't think that will work - the button is initially hidden, so it won't receive style-changed signals until shown; at the same time, its height is used when calculating the window clone's position.

I may not particularly like the current patch, but I'm not sure about there being a cleaner (and less intrusive!) solution ...
Comment 15 Owen Taylor 2010-02-22 16:45:28 UTC
Review of attachment 154382 [details] [review]:

The question of whether parentActor was in the stage or not was actually why I didnt' suggest the approach originally. I didn't realize that we were being called both with and without parentActor being "anchored" to a stage so I didn't think a conditional would actually be needed, but without the conditional there was some "fragility" of the code that I didn't like - if things got rearranged it might break.

I think it's fine as you have it. Not absolutely beautiful but compact and clear :-)
Comment 16 Florian Müllner 2010-02-22 17:23:54 UTC
Comment on attachment 154382 [details] [review]
Fix invalid call to st_widget_get_theme_node() in WindowOverlay

Attachment 154382 [details] pushed as 126d02a - Fix invalid call to st_widget_get_theme_node() in WindowOverlay
Comment 17 Colin Walters 2010-03-08 17:37:51 UTC
Review of attachment 154079 [details] [review]:

I'd like to go with making them fatal for now; let's apply this.
Comment 18 Dan Winship 2010-03-10 14:30:13 UTC
Attachment 154079 [details] pushed as d128cc5 - st_widget_get_theme_node: g_error if called on an unparented widget
Comment 19 Emmanuele Bassi (:ebassi) 2010-04-07 14:09:10 UTC
I have a slight objection to the "let's just crash the shell" approach. when using a gnome-shell from a package then you're most likely never going to see the g_error() message - nor any other message, ever.

it's also pretty clear that just crashing with a g_error() hasn't helped at all avoiding committing patches that end up calling get_theme_node() on unparented actors, since a month after this particular course of action was chosen and committed I have a shell that crashes every time I get into the chrome.

one option that wouldn't be the equivalent of carpet bombing the user would be to use a g_critical() and have the shell run using a g_log_fatal_mask() of G_LOG_LEVEL_CRITICAL when running from a jhbuild root.
Comment 20 Emmanuele Bassi (:ebassi) 2010-04-07 14:11:36 UTC
failing the fatal-critical approach, at least the g_error() should print out more debugging information, e.g.:

  - the widget for which the style was asked (name and/or type)
  - the parent widget, if any (name and/or type)

otherwise the usefulness in debugging is severely limited
Comment 21 Florian Müllner 2013-06-24 18:54:41 UTC
Created attachment 247670 [details] [review]
st: Be more forgiving when calling get_theme_node() on unstaged widgets

While it is obviously still an error to call get_theme_node() on a
widget that hasn't been added to the stage hierarchy yet, asserting
on it hasn't proven too successful in avoiding those errors - it's
likely the most frequent reason for crash reports. Just accept that
there'll always be code paths where we can hit this case and make
it non-fatal.
Comment 22 Jasper St. Pierre (not reading bugmail) 2013-06-24 19:07:41 UTC
Review of attachment 247670 [details] [review]:

OK.
Comment 23 Florian Müllner 2013-06-24 19:55:20 UTC
Attachment 247670 [details] pushed as e70c0d3 - st: Be more forgiving when calling get_theme_node() on unstaged widgets
Comment 24 drago01 2013-06-24 21:09:26 UTC
I went ahead and cherry picked this into 3.8 ... we shouldn't let users wait for another six months to finally get rid of this crashes.
Comment 25 Florian Müllner 2013-06-26 17:20:01 UTC
*** Bug 682383 has been marked as a duplicate of this bug. ***
Comment 26 Peter 2013-06-29 16:47:13 UTC
Thanks Florian! This "prevents" the crashing of Nautilus if started as first application.
Comment 27 Jeremy Bicha 2013-07-03 16:01:13 UTC
*** Bug 695030 has been marked as a duplicate of this bug. ***