598458 - Crash running testAccessPageInfoDialog.js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[cmtalbert]

Attachment: patch to correct the assertion

While working to turn on the new Mozmill unit tests, we found a crash on windows 7 debug builds.  

We were able to figure out it was the testAccessPageInfoDialog.js test: 
http://mxr.mozilla.org/mozilla-central/source/testing/mozmill/tests/firefox/testTechnicalTools/testAccessPageInfoDialog.js

You can run this from a packaged test by (from objdir/dist/test-package-stage/mozmill):
$> python installmozmill.py
$> Scripts/mozmill.exe -t tests/testTechnicalTools -b ../../bin/firefox.exe

Mrbkap took a look at this and figured out the culprit is a bad assertion.  His patch is attached.  And he noted this is likely a regression from bug 595668.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #0)
> While working to turn on the new Mozmill unit tests, we found a crash on
> windows 7 debug builds.  

Looks great that we got a patch that fast. Looking forward which other tests causes stability issues and test failures with debug builds.

Comment 2

[cmtalbert]

One note, you'll need this patch to run the installmozmill script: https://bug516984.bugzilla.mozilla.org/attachment.cgi?id=477279

That change should get landed later today.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 477297 [details] [diff] [review]
patch to correct the assertion

Thanks!

Comment 4

[cmtalbert]

(In reply to comment #3)
> Comment on attachment 477297 [details] [diff] [review]
> patch to correct the assertion
> 
> Thanks!

What does the timeframe for landing this look like?  If it needs to wait until we tag beta 7, then I need to let RelEng know.  RelEng is planning to go live with Mozmill unit tests on Thursday, but we don't want to go live with a perma-orange, which is what would happen without this patch.  Let me know.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #4)
> What does the timeframe for landing this look like?  If it needs to wait until
> we tag beta 7, then I need to let RelEng know.  RelEng is planning to go live
> with Mozmill unit tests on Thursday, but we don't want to go live with a
> perma-orange, which is what would happen without this patch.  Let me know.

Lets ask Beltzner and Johnathan.

Comment 6

[Johnathan Nightingale [:johnath]]

Don't have a good answer for you, Henrik - it's not clear what the question really is. We have what appears to be a test-fix patch, but it touches live code and is not a b7 blocker. By that metric, then, I'd say it's unlikely to land before b7 is tagged - but if that decision hurts us in other ways, it can be revisited. I just need to understand better what the decision is we're being asked to make.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #6)
> be revisited. I just need to understand better what the decision is we're being
> asked to make.

Jeff and Clint are working on the buildbot integration for Mozmill, so a subset of our existing tests can get run on buildbot starting hopefully today or early next week. One of those selected tests for the initial testsuite triggers this crash. So we have two options:

1. Getting this patch landed for b7 and we can execute the complete testsuite
2. We have to remove this single test from the testsuite until this bug has been fixed

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 477297 [details] [diff] [review]
patch to correct the assertion

Let's just get this in.

Comment 9

[David Mandelin [:dmandelin]]

Setting assignee so this doesn't look like an unassigned blocker.

Comment 10

[Nochum Sossonko [:Natch]]

fixed-in-tracemonkey?

Comment 11

[cmtalbert]

This is assigned to me, but I'm not sure what you need me to do for it.  Please let me know what you expect.

Comment 12

[Andreas Gal :gal]

Is this checked in?

Comment 13

[Andreas Gal :gal]

Minor, debug mode assert only.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Crashes are always marked as critical, and given by comment 0 we have a crash, right?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

I'm going to check this into tracemonkey right now. It's a fatal assertion in debug builds only. We can leave this as critical for now.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Actually, all of this code went away with compartments.