Bug 153194 – REGRESSION(r192773): [GTK] maps.google.com unresponsive/stalls since r192773

• Component: WebKitGTK
• Status: RESOLVED FIXED

Going to maps.google.com results in a default map being shown and then the impossibility to drag the map or input a location during or after loading. Resizing the window shows no paint updates for the particular tab in epiphany, however the tab can be closed (just takes a while).

As suggested on IRC I've built webkit both with and without opengl enabled, this made no difference.

[Guilaume Ayoub]

I can confirm this bug, happening for example on:
- http://maps.google.com
- https://www.google.com/recaptcha/api2/demo after clicking on the recaptcha
- https://reviewable.io/reviews/kozea/weasyprint/291#-:-K7vq1bdt8Ke36KDOXBW:-1975054978

These pages are rendered correctly for me with 2.11.1.

[Guilaume Ayoub]

2.11.4 doesn't fix this bug.

[Michael Catanzaro]

*** Bug 153302 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

The "good" news is that this got backported to 2.10, which should make it easier to find out which commit is to blame.

The bad news is that this got backported. Honestly I think we are being a bit too aggressive with the number of commits backported to stable....

[Carlos Garcia Campos]

(In reply to comment #4)
> The "good" news is that this got backported to 2.10, which should make it
> easier to find out which commit is to blame.
> 
> The bad news is that this got backported. Honestly I think we are being a
> bit too aggressive with the number of commits backported to stable....

I don't think it's a matter of number of commits, we had a lot of commits in this release because it took a bit more time since the laste release. All commits landed are bug fixes, rendering issues, crashes and security bugs. What should I leave out? I always try to avoid major refactorings, or large changes in JSC. For every commit I merge, if it's JSC I run all the javascript tests, if it's a rendering issue or crash referencing layout tests I run those, and if it affects the GTK API I run the GTK API tests. I could still merge a commit that breaks something or regresses, of course, but that wouldn't change if I merged fewer commits.

[Andres Gomez Garcia]

(In reply to comment #5)
...
> I don't think it's a matter of number of commits, we had a lot of commits in
> this release because it took a bit more time since the laste release. All
> commits landed are bug fixes, rendering issues, crashes and security bugs.
> What should I leave out? I always try to avoid major refactorings, or large
> changes in JSC. For every commit I merge, if it's JSC I run all the
> javascript tests, if it's a rendering issue or crash referencing layout
> tests I run those, and if it affects the GTK API I run the GTK API tests. I
> could still merge a commit that breaks something or regresses, of course,
> but that wouldn't change if I merged fewer commits.

I agree it is not the number of commits but maybe the criteria could be slightly changed.

What about only merging:
* fixes of bugs reported to the stable branch.
* security fixes
* other kind of fixes that have been already merged in (current_unstable_version - 1) and no regressions have been reported on them.

WDYT?

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> ...
> > I don't think it's a matter of number of commits, we had a lot of commits in
> > this release because it took a bit more time since the laste release. All
> > commits landed are bug fixes, rendering issues, crashes and security bugs.
> > What should I leave out? I always try to avoid major refactorings, or large
> > changes in JSC. For every commit I merge, if it's JSC I run all the
> > javascript tests, if it's a rendering issue or crash referencing layout
> > tests I run those, and if it affects the GTK API I run the GTK API tests. I
> > could still merge a commit that breaks something or regresses, of course,
> > but that wouldn't change if I merged fewer commits.
> 
> I agree it is not the number of commits but maybe the criteria could be
> slightly changed.
> 
> What about only merging:
> * fixes of bugs reported to the stable branch.
> * security fixes
> * other kind of fixes that have been already merged in
> (current_unstable_version - 1) and no regressions have been reported on them.
> 
> WDYT?

That doesn't fix anything either, because for example in this particular case we don't know which commit in trunk broke maps.google.com. I prefer to fix 10 issues and introduce 1 regression than fixing 5 issues with no regressions. In any case I always try to avoid merging commits that have been recently committed in trunk, so I'm doing something similar already in the end.

[Andres Gomez Garcia]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > ...
> > > I don't think it's a matter of number of commits, we had a lot of commits in
> > > this release because it took a bit more time since the laste release. All
> > > commits landed are bug fixes, rendering issues, crashes and security bugs.
> > > What should I leave out? I always try to avoid major refactorings, or large
> > > changes in JSC. For every commit I merge, if it's JSC I run all the
> > > javascript tests, if it's a rendering issue or crash referencing layout
> > > tests I run those, and if it affects the GTK API I run the GTK API tests. I
> > > could still merge a commit that breaks something or regresses, of course,
> > > but that wouldn't change if I merged fewer commits.
> > 
> > I agree it is not the number of commits but maybe the criteria could be
> > slightly changed.
> > 
> > What about only merging:
> > * fixes of bugs reported to the stable branch.
> > * security fixes
> > * other kind of fixes that have been already merged in
> > (current_unstable_version - 1) and no regressions have been reported on them.
> > 
> > WDYT?
> 
> That doesn't fix anything either, because for example in this particular
> case we don't know which commit in trunk broke maps.google.com. I prefer to
> fix 10 issues and introduce 1 regression than fixing 5 issues with no
> regressions. In any case I always try to avoid merging commits that have
> been recently committed in trunk, so I'm doing something similar already in
> the end.

Obviously, you will always will have the risk of introducing regressions. That's never guaranteed. The question is which is the priority in stable and I disagree with you. You should try as hard as possible to avoid introducing regressions so I would favor fixing 5 issues with no regressions than the other way around.

Anyway, just my opinion.

[Michael Catanzaro]

By any chance, is there anyone at the office with ICC who would be willing to bisect this between 2.10.5 and 2.10.4?

[Andres Gomez Garcia]

(In reply to comment #9)
> By any chance, is there anyone at the office with ICC who would be willing
> to bisect this between 2.10.5 and 2.10.4?

I'm getting to it ...

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > ...
> > > > I don't think it's a matter of number of commits, we had a lot of commits in
> > > > this release because it took a bit more time since the laste release. All
> > > > commits landed are bug fixes, rendering issues, crashes and security bugs.
> > > > What should I leave out? I always try to avoid major refactorings, or large
> > > > changes in JSC. For every commit I merge, if it's JSC I run all the
> > > > javascript tests, if it's a rendering issue or crash referencing layout
> > > > tests I run those, and if it affects the GTK API I run the GTK API tests. I
> > > > could still merge a commit that breaks something or regresses, of course,
> > > > but that wouldn't change if I merged fewer commits.
> > > 
> > > I agree it is not the number of commits but maybe the criteria could be
> > > slightly changed.
> > > 
> > > What about only merging:
> > > * fixes of bugs reported to the stable branch.
> > > * security fixes
> > > * other kind of fixes that have been already merged in
> > > (current_unstable_version - 1) and no regressions have been reported on them.
> > > 
> > > WDYT?
> > 
> > That doesn't fix anything either, because for example in this particular
> > case we don't know which commit in trunk broke maps.google.com. I prefer to
> > fix 10 issues and introduce 1 regression than fixing 5 issues with no
> > regressions. In any case I always try to avoid merging commits that have
> > been recently committed in trunk, so I'm doing something similar already in
> > the end.
> 
> Obviously, you will always will have the risk of introducing regressions.
> That's never guaranteed. The question is which is the priority in stable and
> I disagree with you.

I don't think that's the question, the question is whether I could have avoided the regression using a different merge strategy.

> You should try as hard as possible to avoid introducing
> regressions so I would favor fixing 5 issues with no regressions than the
> other way around.

I already try as hard as possible to avoid introducing regressions, the fact that I prefer to fix 10 bugs instead of 5 and introduce 1 regression doesn't mean I'm careless when merging commits. And the key point, again is that I couldn't have avoided this regression just by being more careful. The only way to be more careful is having a bot that runs all the tests for every commit merged in the stable branch, but that would be a lot more work, and it woulnd't be enough either. Again this regression was not caught by our bots in trunk either. And of course I can't run all the tests for every merge myself, 2.10.5 took me 3 days full time merging commits, so I would need 3 weeks to do the same running the tests.

> Anyway, just my opinion.

[Andres Gomez Garcia]

(In reply to comment #11)
...
> I already try as hard as possible to avoid introducing regressions, the fact
> that I prefer to fix 10 bugs instead of 5 and introduce 1 regression doesn't
> mean I'm careless when merging commits. And the key point, again is that I
> couldn't have avoided this regression just by being more careful. The only
> way to be more careful is having a bot that runs all the tests for every
> commit merged in the stable branch, but that would be a lot more work, and
> it woulnd't be enough either. Again this regression was not caught by our
> bots in trunk either. And of course I can't run all the tests for every
> merge myself, 2.10.5 took me 3 days full time merging commits, so I would
> need 3 weeks to do the same running the tests.

I think we are talking about different things. You are talking about running tests and I'm talking about having a space of time in which the changes introduced by the fixes are tested by real users.

That scenario I can only see that happening is if the changes are first introduced in an unstable release and enough time is left until being also merged in a stable release.

And I know that doing so doesn't guarantee that regression won't still be introduced but I think it is a change in the merging policy that can help a lot to prevent that.

[Andres Gomez Garcia]

(In reply to comment #10)
> (In reply to comment #9)
> > By any chance, is there anyone at the office with ICC who would be willing
> > to bisect this between 2.10.5 and 2.10.4?
> 
> I'm getting to it ...

At least, int he 2.10 series, the regression was introduced by:
https://trac.webkit.org/changeset/195046

[Michael Catanzaro]

Thank you Andres!

I don't think more time in the unstable series would have helped; we had two users report this was broken in unstable, so clearly there was enough time in unstable.

Carlos, I think a different merge strategy might have helped in this case indeed. I would never have guessed "[GLIB] Implement garbage collector timers" would have introduced a bug, but when I was looking at the backports a few days ago I noted this one in particular and was confused why it was selected, since it's a large amount of code not fixing any particular issue, even though it looked probably harmless.

I would have avoided the RenderThemeGtk/ScrollbarThemeGtk changes (probably harmless, but huge code changes with no benefit to users), the recent XSSAuditor changes (pushed quite recently, would have held off at least until 2.10.6), Martin's hyphenation fix (probably harmless, but enabling a new feature that we've never supported before), the MIPS fixes and OS X build fixes (just to have fewer commits in stable)....

Still, this is art and not science, the only way to avoid regressions is to not backport anything. Thank you for the hard work on the stable release; I am excited in particular for the scrollbar fix!

[Andres Gomez Garcia]

(In reply to comment #14)
> Thank you Andres!
> 
> I don't think more time in the unstable series would have helped; we had two
> users report this was broken in unstable, so clearly there was enough time
> in unstable.

With more time a bisect could have already been done in unstable to find the guilty commit and not having chosen it for merge in stable.

Anyway, I agree in the rest of what you say. I think it would be better to omit fixes that are not a clear benefit in stable and involve big changes.

[Carlos Garcia Campos]

(In reply to comment #14)
> Thank you Andres!

Yes, thank you!

> I don't think more time in the unstable series would have helped; we had two
> users report this was broken in unstable, so clearly there was enough time
> in unstable.
> 
> Carlos, I think a different merge strategy might have helped in this case
> indeed. I would never have guessed "[GLIB] Implement garbage collector
> timers" would have introduced a bug, but when I was looking at the backports
> a few days ago I noted this one in particular and was confused why it was
> selected, since it's a large amount of code not fixing any particular issue,
> even though it looked probably harmless.

I also had doubts when I merged it, but I remembered that right after landing this in trunk the perf bot stopped crashing, and I also considered this harmless enough to merge it, just in case it fixed anything and assuming it didn't break anything. And I was wrong, but again, I would do it again. It's not that I merged this without even thinking about it, I did, and I ran the tests, but unfortunately it broke a single website.

> I would have avoided the RenderThemeGtk/ScrollbarThemeGtk changes (probably
> harmless, but huge code changes with no benefit to users),

Well, if being able to render forms with GTK+ 2.20 doesn't benefit the users...

> the recent
> XSSAuditor changes (pushed quite recently, would have held off at least
> until 2.10.6), 

Security fixes, no, those are high priority for me. That would work I had time to make stable releases every 2 or 3 weeks, but that's not the case, so security fixes always go in.

> Martin's hyphenation fix (probably harmless, but enabling a
> new feature that we've never supported before),

Exactly because we released 2.10 claiming a feature that doesn't work, we should definitely fix it.

> the MIPS fixes and OS X
> build fixes (just to have fewer commits in stable)....

I guess MIPS and OSX users won't agree with you here either.

> Still, this is art and not science, the only way to avoid regressions is to
> not backport anything. Thank you for the hard work on the stable release; I
> am excited in particular for the scrollbar fix!

[Carlos Garcia Campos]

Created attachment 269745 [details]
Patch

I confirm that this patch also fixes bug #152340 and it could also fix the huge memory leaks reported with some sites, since we were not doing any garbage collection on workers at all before.

[Michael Catanzaro]

Comment on attachment 269745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269745&action=review

Wow, nice job. How did you debug this?

Are you planning a 2.10.6 update soon for this? (I decided to stick with 2.10.4 in Fedora due to this issue.)

> Source/WebCore/ChangeLog:19
> +        schdule their sources. And then we need to check if there are

schdule -> schedule

> Source/WebCore/workers/WorkerRunLoop.cpp:151
> +    if (g_main_context_pending(g_main_context_get_thread_default()))

This should be:

if (g_main_context_pending(mainContext))

[Michael Catanzaro]

*** Bug 152340 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

(In reply to comment #18)
> Comment on attachment 269745 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269745&action=review
> 
> Wow, nice job. How did you debug this?

With printf, of course! :-D

> Are you planning a 2.10.6 update soon for this? (I decided to stick with
> 2.10.4 in Fedora due to this issue.)

Yes.

> > Source/WebCore/ChangeLog:19
> > +        schdule their sources. And then we need to check if there are
> 
> schdule -> schedule

Good catch.

> > Source/WebCore/workers/WorkerRunLoop.cpp:151
> > +    if (g_main_context_pending(g_main_context_get_thread_default()))
> 
> This should be:
> 
> if (g_main_context_pending(mainContext))

Right, forgot to change this. Thanks!

[Carlos Garcia Campos]

Committed r195537: <http://trac.webkit.org/changeset/195537>

[Zan Dobersek]

Comment on attachment 269745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269745&action=review

> Source/WebCore/workers/WorkerRunLoop.cpp:152
> +    if (g_main_context_pending(g_main_context_get_thread_default()))
> +        g_main_context_iteration(mainContext, FALSE);

Is a single iteration enough? Usually this duo works together in a while statement.

[Carlos Garcia Campos]

(In reply to comment #22)
> Comment on attachment 269745 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269745&action=review
> 
> > Source/WebCore/workers/WorkerRunLoop.cpp:152
> > +    if (g_main_context_pending(g_main_context_get_thread_default()))
> > +        g_main_context_iteration(mainContext, FALSE);
> 
> Is a single iteration enough? Usually this duo works together in a while
> statement.

Yes, any other source pending will be handled in the next iteration. I tried to follow the one iteration, one source rule. If this causes any problem we can dispatch all pending sources in the same iteration.