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 683029 - Crash trying the in-tab-overview branch
Crash trying the in-tab-overview branch
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Claudio Saavedra
Epiphany Maintainers
: 683478 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-08-30 12:19 UTC by Giovanni Campagna
Modified: 2012-09-17 17:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
EphyOverviewStore: don't register the snapshot mtime as long (1.21 KB, patch)
2012-09-06 15:44 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2012-08-30 12:19:25 UTC
Stack trace:

  • #0 g_object_unref
    at gobject.c line 2915
  • #1 ephy_overview_store_needs_snapshot
    at ephy-overview-store.c line 529
  • #2 on_find_urls_cb
    at ephy-frecent-store.c line 80
  • #3 on_find_urls_cb
    at ephy-frecent-store.c line 43
  • #4 ephy_history_service_execute_job_callback
    at ephy-history-service.c line 470
  • #5 g_main_dispatch
    at gmain.c line 2707
  • #6 g_main_context_dispatch
    at gmain.c line 3211
  • #7 g_main_context_iterate
    at gmain.c line 3282
  • #8 g_main_context_iteration
    at gmain.c line 3343
  • #9 g_application_run
    at gapplication.c line 1607
  • #10 main
    at ephy-main.c line 493

Epiphany version: 3.5.90-56-g155260a
WebKit: 3eca51fb05a979427068f464c1db6fa4766bda4a (1.11.0)
Comment 1 Claudio Saavedra 2012-08-30 13:35:49 UTC
What were you doing when this happened?
Comment 2 Giovanni Campagna 2012-08-30 16:04:00 UTC
Uhm, I'm not sure. I happened a few times, but it's not really reproducibile.
I got it again, by opening and closing the overview multiple times, then accessing a tile that was fully white.
Comment 3 Claudio Saavedra 2012-08-31 17:16:18 UTC
I pushed a fix for a crasher that could be related (bug 683040). Could you see if you ever manage to reproduce this again?
Comment 4 Giovanni Campagna 2012-09-01 15:38:38 UTC
Yes, reproduced again, on current master (3.5.90-87-g525ad1b)

New trace:
  • #0 g_object_unref
    at gobject.c line 2915
  • #1 ephy_overview_store_needs_snapshot
    at ephy-overview-store.c line 483
  • #2 on_find_urls_cb
    at ephy-frecent-store.c line 85
  • #3 on_find_urls_cb
    at ephy-frecent-store.c line 43
  • #4 ephy_history_service_execute_job_callback
    at ephy-history-service.c line 472
  • #5 g_main_dispatch
    at gmain.c line 2715
  • #6 g_main_context_dispatch
    at gmain.c line 3219
  • #7 g_main_context_iterate
    at gmain.c line 3290
  • #8 g_main_context_iteration
    at gmain.c line 3351
  • #9 g_application_run
    at gapplication.c line 1607
  • #10 main
    at ephy-main.c line 496

Comment 5 Claudio Saavedra 2012-09-04 19:22:49 UTC
So I did some rework in the code handling the thumbnails in the model that might have tackled this, but I have no certainty whatsoever. All I know is that I've been running ephy under gdb for a while now and I am unable to make it crash.

Of course if you have more precise instructions on how to reproduce it, that would really help.
Comment 6 Giovanni Campagna 2012-09-06 00:23:00 UTC
And again and again! This time, either it fails before fully loading the session, or just going to the "Oops, this page might have crashed the browser" and clicking "reload again" triggers it.
Same trace.

Epiphany is 3.5.91.1-5-gb07db6a, WebKit is 69cc15fb104158eca60725b8f926fa984ddd6704.
Comment 7 Giovanni Campagna 2012-09-06 00:26:52 UTC
Oh, and by the way, the problem now exists for application mode too. There, the browser crashes midway through the page load.
Comment 8 Giovanni Campagna 2012-09-06 00:56:19 UTC
Interesting, I blew the thumbnails cache and the problem did not appear. I restarted epiphany and it promptly crashed, so the problem seems to be in the caching code.
Comment 9 Claudio Saavedra 2012-09-06 15:10:19 UTC
*** Bug 683478 has been marked as a duplicate of this bug. ***
Comment 10 Claudio Saavedra 2012-09-06 15:11:52 UTC
Priit bisected this and found that the first commit where this is reproducible is c02832ac1f68cbdcf3fe97348983a64455137d8e.
Comment 11 Giovanni Campagna 2012-09-06 15:19:10 UTC
That commit has a bug on x86_64: it uses int for the mtime, but it registers it as long.
I changed the GType to int, and I hasn't crashed yet.
Comment 12 Claudio Saavedra 2012-09-06 15:39:51 UTC
OK, that would actually make sense. Could you please submit your patch for others to try?
Comment 13 Giovanni Campagna 2012-09-06 15:44:05 UTC
Created attachment 223666 [details] [review]
EphyOverviewStore: don't register the snapshot mtime as long

All code paths use int, and this breaks on architechtures where
long is not the same as int (such as x86_64).
Comment 14 Claudio Saavedra 2012-09-06 15:48:52 UTC
hm, I was thinking, isn't time_t supposed to be int64? I wonder if the proper fix shouldn't be the other way around (using long everywhere).
Comment 15 Priit Laes (IRC: plaes) 2012-09-06 15:52:29 UTC
Isn't there similar issue with EPHY_OVERVIEW_STORE_LAST_VISIT ?
Comment 16 Priit Laes (IRC: plaes) 2012-09-06 15:58:05 UTC
Patch works great!
Comment 17 Xan Lopez 2012-09-06 17:50:04 UTC
(In reply to comment #13)
> Created an attachment (id=223666) [details] [review]
> EphyOverviewStore: don't register the snapshot mtime as long
> 
> All code paths use int, and this breaks on architechtures where
> long is not the same as int (such as x86_64).

Sorry if I'm dense, but can you explain why exactly this breaks in x86_64?
Comment 18 Claudio Saavedra 2012-09-06 18:04:58 UTC
(In reply to comment #15)
> Isn't there similar issue with EPHY_OVERVIEW_STORE_LAST_VISIT ?

Yes. This might work because we're not really using that column right now.

About the patch, I am unsure that using int is the right thing to do. As far as
I understand time_t is not necessarily an int. I think that a better approach
would be to use time_t everywhere and a big enough type for the model
(G_TYPE_LONG seems right).
Comment 19 Giovanni Campagna 2012-09-06 19:08:26 UTC
(In reply to comment #17)
> (In reply to comment #13)
> > Created an attachment (id=223666) [details] [review] [details] [review]
> > EphyOverviewStore: don't register the snapshot mtime as long
> > 
> > All code paths use int, and this breaks on architechtures where
> > long is not the same as int (such as x86_64).
> 
> Sorry if I'm dense, but can you explain why exactly this breaks in x86_64?

GtkTreeModel/GtkListStore uses varargs, and they depend on the length on the argument. If a column is typed G_TYPE_LONG, on set 64 bits of argument (or one full register, in x86_64) are read, while on get, the pointer is expected to hold 64 bits. On the other hand, the default argument promotions only go up to int, so undefined data read and/or stack corruption can happen on set. Worse, and probably the cause of this bug, on get it's possible that another adjacent variable is overwritten.
Comment 20 Claudio Saavedra 2012-09-06 19:38:29 UTC
commit 8431cabd4d8f72dfaadbe64adf9f8dc89a26ae62
Author: Giovanni Campagna <gcampagna@src.gnome.org>
Date:   Thu Sep 6 17:40:51 2012 +0200

    EphyOverviewStore: don't register the snapshot mtime as long
    
    All code paths use int, and this breaks on architechtures where
    long is not the same as int (such as x86_64).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=683029
    
    [Do the same for the visit time column]
    Signed-off-by: Claudio Saavedra <csaavedra@igalia.com>


Attachment 223666 [details] pushed as 8431cab - EphyOverviewStore: don't register the snapshot mtime as long
Comment 21 Bastien Nocera 2012-09-17 17:14:28 UTC
Make sure this gets into a 3.5.92 release, it's making ephy unusable for me.