573255 - Display of failure summary and suggestions broken on tbpl

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Phil Ringnalda (:philor)]

STR:

1. In any build including the push topped with http://hg.mozilla.org/mozilla-central/rev/d4156799e66a, open http://tests.themasta.com/tinderboxpushlog/
2. Find an unstarred red or orange letter/number on the right side and click it
3. Wait for "Retrieving summary" to go away, and notice that instead of a blue-background popup listing the errors from the brief log and suggested possible bugs, you've got absolutely no popup visible.

Works with builds up through http://hg.mozilla.org/mozilla-central/rev/88fa0b783306 (the push before the d4156799e66a push).

(BLO might be a little strong for severity, but in my world, no developer would be able to go past 88fa0b783306 because every developer would need to be able to see build errors because they would be starring them.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The range there is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=88fa0b783306&tochange=d4156799e66a

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm working on bisecting further.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/a6e3300a3bac seems to be the guilty changeset.

Comment 4

[Phil Ringnalda (:philor)]

Don't think tbpl is in any mxrs, but the repo is http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My guess is that the relevant class would be .hasSummary, which is used in selectors only inside :not().

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: simple testcase

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, I understand what's happening now (I should have seen it sooner), but I don't see how to fix it:
 + before the attribute change, the class attribute has no classes in it, so we don't do any hashtable lookup
 + after the attribute change, the class attribute has the class in it, but the selector does not match

My inclination would be to back out the patch.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... back out the patch and add a test, that is.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, I think there is another possibility:  not call AttributeEnumFunc at all, but at the point we do, just presume that there is attribute-dependent style.

It's not obvious to me whether that would be better or worse than the approach prior to bug 571347; there are plenty of cases both ways that one optimization optimizes for and the other misses.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Some other options:
 * we could add a "class value to be ignored" argument to SelectorMatches
 * we could temporarily lop off the mNegations before calling AttrMatchesValue... though that would require also doing so inside :-moz-any()

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out the cause:
http://hg.mozilla.org/mozilla-central/rev/284711070f31
http://hg.mozilla.org/mozilla-central/rev/2f5b0483c339
and added a test:
http://hg.mozilla.org/mozilla-central/rev/db2d093fdd57
(in which I used visibility rather than display because it's using computed style).

Comment 12

[Boris Zbarsky [:bzbarsky]]

David, thanks for backing that out.  I'll follow up in bug 571347.