Closed Bug 1126295 Opened 5 years ago Closed 4 years ago

Move TestAtoms.cpp to gtest and enable it

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ehsan, Assigned: sajitk)

References

Details

Attachments

(2 files)

No description provided.
Attached patch Patch (v1)Splinter Review
Attachment #8555244 - Flags: review?(nfroyd)
Assignee: nobody → ehsan
Comment on attachment 8555244 [details] [diff] [review]
Patch (v1)

Review of attachment 8555244 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/gtest/UTFStrings.h
@@ +17,5 @@
>        { 'a', 'b', 'c', 'd' } },
>      { { '1', '2', '3', '4' },
>        { '1', '2', '3', '4' } },
>      { { 0x7F, 'A', 0x80, 'B', 0x101, 0x200 },
> +      { 0x7F, 'A', char(0xC2), char(0x80), 'B', char(0xC4), char(0x81), char(0xC8), char(0x80) } },

Ah, compiler warnings and the changes they require to the code, how I love them.
Attachment #8555244 - Flags: review?(nfroyd) → review+
These tests are busted, I backed them out:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1422380541/mozilla-inbound-win32-bm82-build1-build5368.txt.gz


TEST-UNEXPECTED-FAIL | Atoms.Table | SEH exception with code 0xc0000005 thrown in the test body. @ (null):-1
TEST-UNEXPECTED-FAIL | Atoms.Table | test completed (time: 0ms)
TEST-UNEXPECTED-FAIL | Atoms.Permanent | SEH exception with code 0xc0000005 thrown in the test body. @ (null):-1
TEST-UNEXPECTED-FAIL | Atoms.Permanent | test completed (time: 0ms)
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | Value of: err
TEST-UNEXPECTED-FAIL | UTF.Hash16 | test completed (time: 4ms)
TEST-UNEXPECTED-FAIL | GTest unit test: failed
gtest TEST-UNEXPECTED-FAIL | gtest | test failed with return code 1
Assignee: ehsan → nobody
This reproduces reliably on try: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=3703d3deac4b>

I have been unable to reproduce locally so far.
Flags: needinfo?(ehsan)
I have been unable to reproduce this locally.  Bug 1136841 makes it impossible to get a stack trace for this crash on the try server, and because we do not package xul-gtest, running the gtests locally from a build produced on the try server doesn't work either.
Depends on: 1136841
Flags: needinfo?(ehsan)
I was able to reproduce the SEH exception on my local non-debug build on Windows 7. I then isolated the exception to the method:

> bool isStaticAtom(nsIAtom* atom)

To isolate the statement in which the crash was happening, I broke up the single statement with & to a series of statements with &=, as in the attached patch.

And the SEH exception went away. I have no idea why the change stopped the crash (related to evaluation order maybe?)

I wanted to run this on the try server, but I was not sure which tests include the gTests. Could you please let me know and I can schedule a run to try.
Attachment #8673829 - Flags: review?(nfroyd)
Comment on attachment 8673829 [details] [diff] [review]
1126295-new.patch

Review of attachment 8673829 [details] [diff] [review]:
-----------------------------------------------------------------

Cool!  Thanks for tracking that down!  The 'B' step on try currently runs gtests, so you're all set as far as confirming your changes are OK.  I'd say that your existing try push is sufficient for checkin-needed.
Attachment #8673829 - Flags: review?(nfroyd) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7cf5a24145ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee: nobody → sajitk
You need to log in before you can comment on or make changes to this bug.