237955 - [FIXr]Filter out NS_BLOCK_HAS_LINE_CURSOR in regression tests

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

I thought about this some more and I may just add a method on nsIFrameDebug that
returns the flags and masks out the irrelevant ones.  The current regression
test logic could then move into that debug method on nsFrame, and nsBlockFrame
can override.

If you think it's better to keep all the logic in the regression test code,
please let me know....

Comment 1

[Boris Zbarsky [:bzbarsky]]

I had another thought here.  Maybe a nsIFrameDebug method to get the flags? 
Then the frames could mask out flags as needed.... thoughts?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

sounds reasonable.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Thusly

Comment 4

[Bernd]

Isn't that a little bit of overkill? (See for
http://www.infiltec.com/j-h-wrld.htm an example where we might end :-)) I did
not see this NS_BLOCK_HAS_LINE_CURSOR over three years of running the tests. And
wouldn't the compare trees a better place to do this instead of adding functions
that only serve the regressions tests to some frames?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I did not see this NS_BLOCK_HAS_LINE_CURSOR over three years of running the
> tests

That's because it got added a few weeks ago.  I got nearly a thousand false
positives on every single regression test I've run since then (that's nearly a
thousand tests showing false positives; probably about 30% of the total tests we
have.  Different ones each time, of course).

> And wouldn't the compare trees a better place to do this

NS_BLOCK_HAS_LINE_CURSOR is in the "reserved for frames" bit-space.  Which means
that bit can be meaningful for frames that are not blockframes.  There is no
reliable way to detect blockframes and all subclasses of blockframes from the
compare trees code.

But if you think there's a better way to implement this, I'm all ears.  I'm not
tied to this particular implementation.

Comment 6

[Bernd]

>That's because it got added a few weeks ago.
Ah, the hidden advantage of having a single directory where other people seldom
checkin. Sorry, I did not know thats so critical, 1000 false positives are far
beyond tolerable problems. With your explanation its OK with me and thanks for
the warning.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Index: layout/base/public/nsIFrameDebug.h
+   * Return the state bits that are relevant to regression tests (that is,
those bits which indicate a real difference when they differ

nit: missing closing ')'

Comment 8

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

Comment on attachment 145305 [details] [diff] [review]
Thusly

This is all DEBUG code only, and I'd like to run the regression tests on
something that should probably go into 1.7, so approving.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.7

Comment 10

[Michael Moy]

Attempted debug build Tuesday evening:

   Creating library firefox.lib and object firefox.exp
gklayout.lib(nsIsIndexFrame.obj) : error LNK2001: unresolved external symbol "pu
blic: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(void)const
" (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsLegendFrame.obj) : error LNK2001: unresolved external symbol "pub
lic: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(void)const "
 (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsMathMLForeignFrameWrapper.obj) : error LNK2001: unresolved extern
al symbol "public: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBit
s(void)const " (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsMathMLmtableFrame.obj) : error LNK2001: unresolved external symbo
l "public: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(void)c
onst " (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsFileControlFrame.obj) : error LNK2001: unresolved external symbol
 "public: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(void)co
nst " (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsSelectsAreaFrame.obj) : error LNK2001: unresolved external symbol
 "public: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(void)co
nst " (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsComboboxControlFrame.obj) : error LNK2001: unresolved external sy
mbol "public: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(voi
d)const " (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsBlockFrame.obj) : error LNK2001: unresolved external symbol "publ
ic: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(void)const "
(?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsAreaFrame.obj) : error LNK2001: unresolved external symbol "publi
c: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(void)const " (
?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsMathMLContainerFrame.obj) : error LNK2001: unresolved external sy
mbol "public: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(voi
d)const " (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
gklayout.lib(nsTableOuterFrame.obj) : error LNK2001: unresolved external symbol
"public: virtual unsigned int __stdcall nsBlockFrame::GetDebugStateBits(void)con
st " (?GetDebugStateBits@nsBlockFrame@@UBGIXZ)
firefox.exe : fatal error LNK1120: 1 unresolved externals
make[4]: *** [firefox.exe] Error 96
make[4]: Leaving directory `/cygdrive/c/Mozilla/mozilla/browser/app'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/c/Mozilla/mozilla/browser'
make[2]: *** [tier_99] Error 2
make[2]: Leaving directory `/cygdrive/c/Mozilla/mozilla'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/cygdrive/c/Mozilla/mozilla'
make: *** [build] Error 2

Comment 11

[Boris Zbarsky [:bzbarsky]]

Sounds like you're linking together libs that were built with different build
flags or something.  Is this only an issue for the static build?

Comment 12

[Michael Moy]

Yes, I generally only do static builds. Would changing the build to shared fix this problem? I'm just trying to get something to work at the moment.

Comment 13

[Boris Zbarsky [:bzbarsky]]

No idea, but it's worth trying.  Again, it's looking to me like some differing
versions of libs are being statically linked there.

Comment 14

[Michael Moy]

It looks like the function prototype was missing or not being seen as it appears to be defaulting to a type of int. Has anyone else had any problems with debug
builds on FireFox? Or does anyone do them?

At any rate, my build should be done in a few hours.

Comment 15

[Boris Zbarsky [:bzbarsky]]

nsFrameState _is_ an int.  "virtual unsigned int __stdcall" is the correct
return type for that function on Windows.

Comment 16

[Michael Moy]

Build succeeded but it crashes on startup in JSAPI.C on an assertion doing something with the maximum number of threads. I guess I'll have to file another bug on that one as it's a new one for me.

How do you guys do debugging? Do you have a debug image that works somewhere?

Comment 17

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

http://www.mozilla.org/community/developer-forums.html#mozilla-builds is a much
better forum than this bug for build questions

Comment 18

[Boris Zbarsky [:bzbarsky]]

Michael, don't you think I maybe compiled and tested this patch to debug-only
code in a debug build before checking it into the tree and all?

Comment 19

[Michael Moy]

(In reply to comment #17)
> http://www.mozilla.org/community/developer-forums.html#mozilla-builds is a much
> better forum than this bug for build questions

I hang out on the FireFox build board and generally post problems like this. When it comes to debug builds, there may be one or two others out there that
have expressed interest in these for debugging problems but most folks say
nothing in response to problems that you have for debug builds. There are lots
of times when you won't get much response for regular builds. Some of the
regular builders get tired and sometimes you have to spend a few days of
hand-holding to get someone through their first build.

(In reply to comment #18)
> Michael, don't you think I maybe compiled and tested this patch to debug-only
> code in a debug build before checking it into the tree and all?

I've worked with hundreds of engineers over the last almost 20 years and I'd
have to say that habits of developers vary widely.

At any rate, debug builds (for the whole project anyways) are broken from my
point of view. All I have available is to file bugs or report it in the build
forums. Both of these approaches haven't yielded much in the way of returns.

That leaves me with changing code or being good at reading assembler in trying
to diagnose problems.

Now it would seem to me that the organization would want as many as possible to have the capability to debug problems on their own.

Especially for those websites that require a membership or subscription cost
making it inconvenient for developers to try to debug these types of problems.

This may be the wrong place to discuss these matters. Perhaps you'd prefer to discuss them on the FireFox build forum. I have a few topics posted on debug
problems in addition to mentioning them in my builds post.

Comment 20

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

DEBUG builds work for everyone else.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Michael, I assume you've done a clobber build?

Comment 22

[Michael Moy]

(In reply to comment #21)
> Michael, I assume you've done a clobber build?

I am unfamiliar with the term.