Bug 128820 - [GTK] Fix hitting hasClass() assertion in debug with the new JS media controls
Summary: [GTK] Fix hitting hasClass() assertion in debug with the new JS media controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords:
: 128904 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-02-14 09:11 PST by Xabier Rodríguez Calvar
Modified: 2014-02-18 08:55 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.57 KB, patch)
2014-02-14 09:18 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (1.86 KB, patch)
2014-02-14 15:12 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xabier Rodríguez Calvar 2014-02-14 09:11:52 PST
[GTK] Fix assertion hit in debug with the new JS media controls
Comment 1 Xabier Rodríguez Calvar 2014-02-14 09:18:12 PST
Created attachment 224223 [details]
Patch
Comment 2 Martin Robinson 2014-02-14 09:22:24 PST
Comment on attachment 224223 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        [GTK] Fix assertion hit in debug with the new JS media controls
> +        https://bugs.webkit.org/show_bug.cgi?id=128820

What assertion is hit?

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * platform/gtk/RenderThemeGtk.cpp:
> +        (WebCore::nodeHasClass): Check for hasClass() in order not to hit

This probably needs a test.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:121
> +    if (node->isElementNode()) {
> +        Element* element = toElement(node);
> +        if (element->hasClass())
> +            return element->classNames().contains(className);
> +    }
> +    return false;

Early returns would be better.
Comment 3 Xabier Rodríguez Calvar 2014-02-14 10:10:25 PST
(In reply to comment #2)
> > Source/WebCore/ChangeLog:4
> > +        [GTK] Fix assertion hit in debug with the new JS media controls
> > +        https://bugs.webkit.org/show_bug.cgi?id=128820
> 
> What assertion is hit?

hasClass(), it is said in the changes explanation. Do you want me to be more explicit? Where exactly?

> > Source/WebCore/ChangeLog:9
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * platform/gtk/RenderThemeGtk.cpp:
> > +        (WebCore::nodeHasClass): Check for hasClass() in order not to hit
> 
> This probably needs a test.

There were quite many tests crashing because of this in debug mode. Actually the bot has an early exit because of those crashes:

Exiting early after 28 crashes and 22 timeouts. 27309 tests run.
121 failures
9 new passes
11 missing results

The results can be seen at:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug%20WK1/r164101%20(6827)/results.html

I think the functionality is quite well covered with the current test set. I could add the info to the changelog instead. Do you still want me to add the test?

> > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:121
> > +    if (node->isElementNode()) {
> > +        Element* element = toElement(node);
> > +        if (element->hasClass())
> > +            return element->classNames().contains(className);
> > +    }
> > +    return false;
> 
> Early returns would be better.

Ok. I'll change this.
Comment 4 Xabier Rodríguez Calvar 2014-02-14 15:12:47 PST
Created attachment 224257 [details]
Patch
Comment 5 WebKit Commit Bot 2014-02-18 01:44:02 PST
Comment on attachment 224257 [details]
Patch

Clearing flags on attachment: 224257

Committed r164277: <http://trac.webkit.org/changeset/164277>
Comment 6 WebKit Commit Bot 2014-02-18 01:44:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Piotr Grad 2014-02-18 08:55:55 PST
*** Bug 128904 has been marked as a duplicate of this bug. ***