Closed Bug 998821 Opened 6 years ago Closed 6 years ago

Handle XShmCreateImage failure without crashing

Categories

(Core :: Widget: Gtk, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(1 file, 1 obsolete file)

This is a little cleaner, avoids doing some unnecessary work if XShm is not available, and makes FF work with rr when usage of XRender is disabled.
Attached patch fix (obsolete) — Splinter Review
Attachment #8409496 - Flags: review?(karlt)
Comment on attachment 8409496 [details] [diff] [review]
fix

>     if (!attachOk || xerror) {
>-        // Assume XShm isn't available, and don't attempt to use it
>-        // again.
>-        gShmAvailable = false;

The presence of the MIT-SHM extension in the server is a necessary condition
to use XShm, but not sufficient, so please leave this code.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)
> This is a little cleaner, avoids doing some unnecessary work if XShm is not
> available,

Skips some work, yes, but adds a round trip to the server.

> and makes FF work with rr when usage of XRender is disabled.

It's reasonable to add code for the sake of rr, but please add a comment
explaining why it is there.

This could also be fixed in rr, by overriding XInitExtension() or
XQueryExtension() instead of XShmQueryExtension() so that all XShm entry
points see that MIT-SHM is not available.
Attachment #8409496 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #2)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)
> > This is a little cleaner, avoids doing some unnecessary work if XShm is not
> > available,
> 
> Skips some work, yes, but adds a round trip to the server.

Oh it does? In that case I shouldn't do this, then.

> This could also be fixed in rr, by overriding XInitExtension() or
> XQueryExtension() instead of XShmQueryExtension() so that all XShm entry
> points see that MIT-SHM is not available.

OK, I'll try that instead. Thanks!
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> > Skips some work, yes, but adds a round trip to the server.
> 
> Oh it does?

Sorry, I checked this out.  XShmQueryExtension() will do a round trip the first time it is called, but XShmAttach() will do this additional round trip if XShmQueryExtension() has not been called, so adding XShmQueryExtension() makes no difference to the number of round trips.

Go ahead with this approach if you like.
Actually I think I'll just override XShmCreateImage in rr as well, making it return null.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Actually, let me repurpose this bug...
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Check for XShm extension before calling XShmCreateImage → Handle XShmCreateImage failure without crashing
Attached patch fixSplinter Review
Attachment #8409496 - Attachment is obsolete: true
Attachment #8409999 - Flags: review?(karlt)
Attachment #8409999 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/229386171230
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.