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)
Core
Widget: Gtk
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41bad8d12f6605f7bf4a6927796a53e96e418797
Assignee | ||
Updated•6 years ago
|
Attachment #8964673 -
Flags: review?(karlt)
Attachment #8964674 -
Flags: review?(bzbarsky)
Comment 4•6 years ago
|
||
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 5•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 6•6 years ago
|
||
(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 7•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 8•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a577c2c20f1882a22628e542081ac27f5f47ef1
Assignee | ||
Comment 9•6 years ago
|
||
(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)
Assignee | ||
Comment 10•6 years ago
|
||
mozreview-review |
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.
Comment 11•6 years ago
|
||
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
Comment 12•6 years ago
|
||
(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
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f5b6a619ca0 https://hg.mozilla.org/mozilla-central/rev/c0eacd0c1662
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•