Closed Bug 1451098 Opened 6 years ago Closed 6 years ago

Calls to gtk_window_resize aren't protected from passing bad arguments

Categories

(Core :: Widget: Gtk, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(2 files)

Sometimes devtools tests are failing with callstacks that indicate failed calls to gtk_window_resize(). One example:

https://treeherder.mozilla.org/logviewer.html#?job_id=168046083&repo=mozilla-inbound&lineNumber=7440

These failures tend to mask root causes. In order to generate better callstacks in such cases, calls to gtk_window_resize() should be protected with asserts.
Attachment #8964673 - Flags: review?(karlt)
Attachment #8964674 - Flags: review?(bzbarsky)
Can you paste crash stacks with the new asserts, please?

Given the AreBoundsSane() checks, I wonder how changing DoResetWidgetBounds() avoids the assertion failure?
Flags: needinfo?(bwerth)
Comment on attachment 8964673 [details]
Bug 1451098 Part 1: Add asserts to widget/gtk/nsWindow.cpp to fail early when setting an invalid window size.

https://reviewboard.mozilla.org/r/233364/#review239078

::: widget/gtk/nsWindow.cpp:4160
(Diff revision 1)
>  
>      LOG(("nsWindow::NativeResize [%p] %d %d\n", (void *)this,
>           size.width, size.height));
>  
>      if (mIsTopLevel) {
> +        MOZ_ASSERT(size.width > 0 && size.height > 0,

The AreBoundsSane() checks above and in NativeMoveResize() should ensure this is not reached, but the assert does check that DevicePixelsToGdkSizeRoundUp() is really rounding up and that overflow is not occurring.

::: widget/gtk/nsWindow.cpp:4944
(Diff revision 1)
>      gint monitorNum = gdk_screen_get_monitor_at_window(screen, gdkWin);
>      GdkRectangle monitorRect;
>      gdk_screen_get_monitor_geometry(screen, monitorNum, &monitorRect);
>      gtk_window_set_screen(gtkWin, screen);
>      gtk_window_move(gtkWin, monitorRect.x, monitorRect.y);
> +    MOZ_ASSERT(monitorRect.width > 0 && monitorRect.height > 0,

G_LOG_LEVEL_CRITICAL is for programming errors and so should trigger
NS_DEBUG_ASSERTION here.

https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/toolkit/xre/nsSigHandlers.cpp#139

That would be a more general solution, rather than trying to catch individual
uses, but that may be harder to land if there are other critical messages that
are not fixed.
Attachment #8964673 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #4)
> Can you paste crash stacks with the new asserts, please?

I can't easily do that at the moment. The failing tests that demonstrated these gtk asserts came from Bug 1435373, which was suffering from bit rot right as I got involved. I had done my own, possibly-incorrect rebasing of the patches in that bug which generated the error log in comment 0. I just tried rebasing those tests again and I can't sort it out. My preferred solution is to have the owner of Bug 1435373 rebase those patches and then be able to apply these patches on top and I'll generate new test logs. I'd rather do that than roll back to a pre-bit-rot build and apply these patches. If the rebasing of Bug 1435373 takes too long or is impossible, I will build the rolled back version and generate the logs from that.
Comment on attachment 8964674 [details]
Bug 1451098 Part 2: Update nsView::DoResetWidgetBounds to check for empty rects returned from CalcWidgetBounds().

https://reviewboard.mozilla.org/r/233366/#review239944
Attachment #8964674 - Flags: review?(bzbarsky) → review+
(In reply to Karl Tomlinson (:karlt) from comment #4)
> Can you paste crash stacks with the new asserts, please?
> 
> Given the AreBoundsSane() checks, I wonder how changing
> DoResetWidgetBounds() avoids the assertion failure?

Ah, I realized I just needed to update my local repository to the original changeset, then start a new try server run with only attachment 8964673 [details] applied on top. Error log is:

https://treeherder.mozilla.org/logviewer.html#?job_id=174394067&repo=try&lineNumber=3815

The call stack does go through DoResetWidgetBounds(), which implies that nsView::CalcWidgetBounds() is returning a bad window size. Attachment 8964674 [details], when applied, accounts for that possibility.

(In reply to Karl Tomlinson (:karlt) from comment #5)
> Comment on attachment 8964673 [details] 
> The AreBoundsSane() checks above and in NativeMoveResize() should ensure
> this is not reached, but the assert does check that
> DevicePixelsToGdkSizeRoundUp() is really rounding up and that overflow is
> not occurring.

With Part 2 applied, it appears that all the calls to gtk_window_resize() are protected, making the MOZ_ASSERTS added in Part 1 unnecessary except for this extremely unlikely scenario. I'd like to still land the patches in case the behavior of DevicePixelsToGdkSizeRoundUp() or DoResetWidgetBounds() changes in a way that makes them unsafe.
Flags: needinfo?(bwerth)
Comment on attachment 8964673 [details]
Bug 1451098 Part 1: Add asserts to widget/gtk/nsWindow.cpp to fail early when setting an invalid window size.

https://reviewboard.mozilla.org/r/233364/#review243520

::: widget/gtk/nsWindow.cpp:4944
(Diff revision 1)
>      gint monitorNum = gdk_screen_get_monitor_at_window(screen, gdkWin);
>      GdkRectangle monitorRect;
>      gdk_screen_get_monitor_geometry(screen, monitorNum, &monitorRect);
>      gtk_window_set_screen(gtkWin, screen);
>      gtk_window_move(gtkWin, monitorRect.x, monitorRect.y);
> +    MOZ_ASSERT(monitorRect.width > 0 && monitorRect.height > 0,

I opened Bug 1455113 to address this.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f5b6a619ca0
Part 1: Add asserts to widget/gtk/nsWindow.cpp to fail early when setting an invalid window size. r=karlt
https://hg.mozilla.org/integration/autoland/rev/c0eacd0c1662
Part 2: Update nsView::DoResetWidgetBounds to check for empty rects returned from CalcWidgetBounds(). r=bz
(In reply to Brad Werth [:bradwerth] from comment #9)
> (In reply to Karl Tomlinson (:karlt) from comment #4)
> > Can you paste crash stacks with the new asserts, please?
> > 
> > Given the AreBoundsSane() checks, I wonder how changing
> > DoResetWidgetBounds() avoids the assertion failure?
> 
> Ah, I realized I just needed to update my local repository to the original
> changeset, then start a new try server run with only attachment 8964673 [details]
> [details] applied on top. Error log is:
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=174394067&repo=try&lineNumber=3815

Thank you.  Your assert is detecting that the code is not doing what I expected.
Filed bug 1455177.
See Also: → 1455177
https://hg.mozilla.org/mozilla-central/rev/4f5b6a619ca0
https://hg.mozilla.org/mozilla-central/rev/c0eacd0c1662
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1503745
You need to log in before you can comment on or make changes to this bug.