Bug 165848 – [GTK] WebProcess from WebKitGtk+ 2.15.2 SIGSEGVs in std::unique_ptr<SoupBuffer, WTF::GPtrDeleter<SoupBuffer> >::get() const () at /usr/include/c++/6/bits/unique_ptr.h:305

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Andres Gomez Garcia]

Created attachment 297087 [details]
BT from gdb

I'm using WebKitGtk+ with my own JHBuild setting:
https://github.com/tanty/jhbuild-epiphany/tree/master

Epiphany 3.20.3 and WebKit 2.15.2 with the attached patches for bug 164049, bug 165200, bug 165283 and bug 164052, applied.

I'm running Epiphany with the dconf key:

"process-model" = "shared-secondary-process"

And the env variable:

"export LIBGL_DRI3_DISABLE=1"

The compilation was done with CMake args:

'-DUSE_LD_GOLD=OFF -DPORT=GTK -DCMAKE_BUILD_TYPE=Release -DENABLE_MINIBROWSER=ON -DCMAKE_C_FLAGS_RELEASE="-O0 -g1 -DNDEBUG -DG_DEBUG=fatal-criticals -DG_DISABLE_CAST_CHECKS" -DCMAKE_CXX_FLAGS_RELEASE="-O0 -g1 -DNDEBUG -DG_DEBUG=fatal-criticals -DG_DISABLE_CAST_CHECKS"'

After visiting several pages, eventually, the WebProcess hits a SIGSEV.

This bug is not reproducible in a predictable way.

[Andres Gomez Garcia]

Created attachment 297186 [details]
Another similar BT from gdb

[Andres Gomez Garcia]

This bug is happening very recurrently.

FTR, the compilation also has '-DENABLE_THREADED_COMPOSITOR=OFF', I forgot to mention that.

[Zan Dobersek]

When you hit this again, in gdb, run these commands:

$ info registers
$ info proc mappings
$ disassemble _ZNKSt10unique_ptrI10SoupBufferN3WTF11GPtrDeleterIS0_EEE3getEv
$ disassemble _ZNK7WebCore12SharedBuffer4dataEv
$ disassemble _ZNK14GIFImageReader4dataEm

The SharedBuffer object on the GIFImageReader might be null.

[Andres Gomez Garcia]

(In reply to comment #3)
> When you hit this again, in gdb, run these commands:
> 
> $ info registers

This is already included in the attached BTs

> $ info proc mappings
> $ disassemble _ZNKSt10unique_ptrI10SoupBufferN3WTF11GPtrDeleterIS0_EEE3getEv
> $ disassemble _ZNK7WebCore12SharedBuffer4dataEv
> $ disassemble _ZNK14GIFImageReader4dataEm

I'll do this.

[Andres Gomez Garcia]

Created attachment 297401 [details]
Yet another similar BT from gdb

This BT additionally includes info for:

$ info proc mappings
$ disassemble _ZNKSt10unique_ptrI10SoupBufferN3WTF11GPtrDeleterIS0_EEE3getEv
$ disassemble _ZNK7WebCore12SharedBuffer4dataEv
$ disassemble _ZNK14GIFImageReader4dataEm

[Andres Gomez Garcia]

Created attachment 297402 [details]
BT from gdb for the UIProcess

After the last SIGSEV, the UIProcess also SIGSEVed. This is its BT.

Have into account that the code was modified accordingly to the suggestion at:
https://bugs.webkit.org/show_bug.cgi?id=165656#c18

[Andres Gomez Garcia]

Created attachment 297416 [details]
And yet another similar BT from gdb

This BT additionally includes info for:

$ info proc mappings
$ disassemble _ZNKSt10unique_ptrI10SoupBufferN3WTF11GPtrDeleterIS0_EEE3getEv
$ disassemble _ZNK7WebCore12SharedBuffer4dataEv
$ disassemble _ZNK14GIFImageReader4dataEm

[Andres Gomez Garcia]

Created attachment 297417 [details]
Another BT from gdb for the UIProcess

After the last SIGSEV, the UIProcess also SIGSEVed. This is its BT.

Have into account that the code was modified accordingly to the suggestion at:
https://bugs.webkit.org/show_bug.cgi?id=165656#c18

[Zan Dobersek]

The hope was that upon the crash the instruction pointer would stay somewhere in _ZNKSt10unique_ptrI10SoupBufferN3WTF11GPtrDeleterIS0_EEE3getEv. That doesn't happen, but the backtrace still reports the crash occurring there.

Assuming the backtrace is correct, with the rdi register having the 0x20 value, it seems that the std::unique_ptr<> getter is called on a std::unique_ptr<> object that's at address 0x20. That's the same value as the offset at which the std::unique_ptr<SoupBuffer> object is positioned inside the SharedBuffer class. So I think the SharedBuffer object in the GIFImageReader class is null.

Also, looking at the memory mappings -- I really want to know what you're loading in the browser, for starters.

[Carlos Garcia Campos]

Maybe this is the same I fixed in r208881

[Andres Gomez Garcia]

(In reply to comment #10)
> Maybe this is the same I fixed in r208881

Sounds like it. I will apply that patch and check ...

[Andres Gomez Garcia]

(In reply to comment #11)
> (In reply to comment #10)
> > Maybe this is the same I fixed in r208881
> 
> Sounds like it. I will apply that patch and check ...

Actually, this is already in 2.15.2

[Andres Gomez Garcia]

(In reply to comment #9)
> Also, looking at the memory mappings -- I really want to know what you're
> loading in the browser, for starters.

I'll try to pass you the crashed session next time it happens.

[Michael Catanzaro]

(In reply to comment #12)
> Actually, this is already in 2.15.2

Yeah, I checked that commit when I saw Zan's comment yesterday. :(

[Andres Gomez Garcia]

Created attachment 297457 [details]
BT from gdb, compiled with -g0

[Andres Gomez Garcia]

Created attachment 297584 [details]
BT from gdb, compiled with -g

[Andres Gomez Garcia]

Created attachment 297585 [details]
BT from gdb for the UIProcess with -g

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Maybe this is the same I fixed in r208881
> > 
> > Sounds like it. I will apply that patch and check ...
> 
> Actually, this is already in 2.15.2

I didn't take into account that size() calls isSizeAvailable() that is virtual, and in the case of GIF, it's overriden and also calls decode(). I think the solution should be to make decoders thread-safe, but in the meantime, could you try moving the LockHolder locker(m_lock); in ImageDecoder::createFrameImageAtIndex() to the beginning? right before the size() call?

[Andres Gomez Garcia]

Created attachment 297900 [details]
BT from gdb with LockHolder moved

I already applied a patch to move LockHolder locker(m_lock); in ImageDecoder::createFrameImageAtIndex() to the beginning

[Andres Gomez Garcia]

Going to this URL and scrolling down reproduces the crash in a 100%

http://wiki.inkscape.org/wiki/index.php/Release_notes/0.92#Important_changes

[Zan Dobersek]

(In reply to comment #20)
> Going to this URL and scrolling down reproduces the crash in a 100%
> 
> http://wiki.inkscape.org/wiki/index.php/Release_notes/0.92#Important_changes

Confirmed with r210274.

[Zan Dobersek]

(In reply to comment #21)
> (In reply to comment #20)
> > Going to this URL and scrolling down reproduces the crash in a 100%
> > 
> > http://wiki.inkscape.org/wiki/index.php/Release_notes/0.92#Important_changes
> 
> Confirmed with r210274.

That's using MiniBrowser. I get a pretty good crash rate if I remove the ~/.cache/MiniBrowser cache before each run.

[Carlos Garcia Campos]

Created attachment 298091 [details]
Patch

Could you please try this new patch?

[Zan Dobersek]

(In reply to comment #23)
> Created attachment 298091 [details]
> Patch
> 
> Could you please try this new patch?

I am not able to reproduce the crash with it, while I can still easily reproduce the crash without the patch applied.

[Andres Gomez Garcia]

Created attachment 298283 [details]
BT from gdb for the WebProcess, with the new patch

[Andres Gomez Garcia]

(In reply to comment #25)
> Created attachment 298283 [details]
> BT from gdb for the WebProcess, with the new patch

Keeps crashing ...

[Andres Gomez Garcia]

Created attachment 298284 [details]
BT from gdb for the UIProcess, with the new patch

Immediately after the SIGSEV in the WebProcess, we have this SIGSEV in the UIProcess.

This is the message in the console:


-- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< --

The program with pid 29593 received an X Window System error.
The error was 'BadDamage (invalid Damage parameter)'.

-- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< --

[Carlos Garcia Campos]

(In reply to comment #25)
> Created attachment 298283 [details]
> BT from gdb for the WebProcess, with the new patch

This bt proves the patch works indeed, main thread is waiting in the mutex, while the decoder thread is the only one decoding the gif image.

[Carlos Garcia Campos]

(In reply to comment #26)
> (In reply to comment #25)
> > Created attachment 298283 [details]
> > BT from gdb for the WebProcess, with the new patch
> 
> Keeps crashing ...

Due to an assert when accessing passed in rowBuffer:

const unsigned char sourceValue = rowBuffer[(m_scaled ? m_scaledColumns[x] : x) - frameContext->xOffset];

it seems that can overflow, which looks unrelated to the threading issue, I think, I would need to look at it in more detail to be sure.

[Carlos Garcia Campos]

(In reply to comment #27)
> Created attachment 298284 [details]
> BT from gdb for the UIProcess, with the new patch
> 
> Immediately after the SIGSEV in the WebProcess, we have this SIGSEV in the
> UIProcess.
> 
> This is the message in the console:
> 
> 
> -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< --
> 
> The program with pid 29593 received an X Window System error.
> The error was 'BadDamage (invalid Damage parameter)'.
> 
> -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< -- 8< --

This is also a different issue.

[Carlos Garcia Campos]

Committed r210501: <http://trac.webkit.org/changeset/210501>