917061 - Fix the last 2 crashtests that use enablePrivilege

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: crashtest.diff

I haven't tested yet if this works, ./mach crashtest doesn't seem to work here, locally.

Comment 1

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=00311cae54eb

Comment 2

[Martijn Wargers (dead)]

It turns out that accessible/tests/crashtests/471493.xul doesn't run under crashtests.
Not sure how they are run on tbpl and tryserver.

Comment 3

[Daniel Holbert [:dholbert]]

The directory would need to be included here:
 http://mxr.mozilla.org/mozilla-central/source/testing/crashtest/crashtests.list
but it's not.

It was listed there for about two hours in March of 2009 -- added here:
 http://hg.mozilla.org/mozilla-central/rev/acdb66fda343
and then removed here:
 http://hg.mozilla.org/mozilla-central/rev/5dacfaef8896
...possibly because it was hanging, because of the enablePrivilege stuff, based on bug 471493 comment 17 / 19

Comment 4

[Daniel Holbert [:dholbert]]

(So: if you want to test the /accessible patch, you'll need to add that line back to testing/crashtest/crashtests.list)

Comment 5

[Martijn Wargers (dead)]

Attachment: 917061.diff

Thanks, I don't see any hangs locally with the accessible test file.
I pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=f9128379e6ef

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 806322 [details] [diff] [review]
917061.diff

>+++ b/layout/mathml/crashtests/400157.xhtml
[...]
>-var navigator1 = parent.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIWebNavigation);
>+var navigator1 = SpecialPowers.wrap(parent).QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIWebNavigation);

In the first test, you did s/Components.interfaces/SpecialPowers.Ci/ -- do we need that here, too? (in the mathml crashtest)

> setTimeout(function() {window.location.reload()}, 500);
> setTimeout(doe,50, 0.2);

So this MathML test has scripted tweaks, which are delayed using setTimeout. That means it should almost certainly be using "reftest-wait", or else we'll likely finish the test and move on before the scripted tweaks fire.

Could you either fix that as well, or file a followup on fixing it?

r=me with that, assuming that:
 (1) the tests pass
 (2) you've verified that we actually succeed in running your added line of script, in each test, when you load the test directly. (e.g. we don't post anything like "Components.interfaces not defined" to the error console)  Otherwise, the tests might just be silently "passing" (loading) because they trip over themselves before they do anything interesting.

Comment 7

[Martijn Wargers (dead)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> In the first test, you did s/Components.interfaces/SpecialPowers.Ci/ -- do
> we need that here, too? (in the mathml crashtest)

Yes! I forgot that. That's the annoying thing with these crashtests/reftests, you don't notice any js errors easily. I'll fix this.

> > setTimeout(function() {window.location.reload()}, 500);
> > setTimeout(doe,50, 0.2);
> 
> So this MathML test has scripted tweaks, which are delayed using setTimeout.
> That means it should almost certainly be using "reftest-wait", or else we'll
> likely finish the test and move on before the scripted tweaks fire.
> 
> Could you either fix that as well, or file a followup on fixing it?

Ok, I'll do that. I guess I should remove the "setTimeout(function() {window.location.reload()}, 500);" call, because that can't work in a crashtest.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #7)
> Ok, I'll do that. I guess I should remove the "setTimeout(function()
> {window.location.reload()}, 500);" call, because that can't work in a
> crashtest.

I suppose; I'm fine either way. It looks like that might've been required to trigger the original bug, so removing that neuters the test; but if it's ineffective, I guess there's no reason to keep it. (And there's probably still some value in keeping the neutered test, as a way to exercise the code involved, at least partway.)

Comment 9

[Martijn Wargers (dead)]

Enabling the accessible crashtest is causing assertions and crashes in the following crashtests, see tryserver: https://tbpl.mozilla.org/?tree=Try&rev=f9128379e6ef
So this crashtest can't be enabled yet, because accessibility code can't handle crashtests.

Comment 10

[Martijn Wargers (dead)]

Attachment: 917061.diff

Ok, I followed your suggestions in comment 6 and comment 8.
I managed to forget another enablePrivilege call in 400157.xhtml, which I removed here also.
I checked the log for weird error messages surrounding this test, but I didn't see any.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 806995 [details] [diff] [review]
917061.diff

Could you file a followup on the assertions/crashes, and CC surkov?

>+++ b/testing/crashtest/crashtests.list
>@@ -1,13 +1,16 @@
> # Add your lines including crashtest manifests below here.
> # DO NOT ADD CRASHTESTS INDIVIDUALLY HERE!
> 
> include ../../testing/crashtest/sanity/crashtests.list
> 
>+# Disabled because it causes assertions and crashes in later crashtests
>+# include ../../accessible/tests/crashtests/crashtests.list
>+

Instead of leaving that "entire" (single-test) crashtests directory commented out, please add it normally (uncommented), and then mark the test as "skip" in its local crashtest manifest, with a comment to its right or above it mentioning your followup bug number.

That way, when someone comes along and adds a second accessibility crashtest to the accessibility crashtests.list file, it'll actually get run instead of silently ignored.

r=me with that

Comment 12

[Martijn Wargers (dead)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Could you file a followup on the assertions/crashes, and CC surkov?

I filed bug 918246 for it.

Comment 13

[Martijn Wargers (dead)]

Attachment: 917061.diff

Ok, done your suggestion in comment 11. Thanks for you help.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 807119 [details] [diff] [review]
917061.diff

>--- a/testing/crashtest/crashtests.list
>+++ b/testing/crashtest/crashtests.list
>+# Disabled because it causes assertions/crashes in later crashtests, see bug 918246
>+skip include ../../accessible/tests/crashtests/crashtests.list
>+

Sorry -- to clarify, I was asking that you just have a normal "include" line there like we do for every other directory, and then put the "skip" annotation *inside* accessible/tests/crashtests/crashtests.list.

That way we *only* skip the specific problematic test (and we *won't* silently skip any other problematic tests that are added in the future)

Comment 15

[Martijn Wargers (dead)]

Attachment: 917061.diff

Ok, done that.
I don't think the test is problematic, btw. It's just that this code turns on accessibility and accessibility can't handle crashtests.

Comment 16

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=90808a5fc93c

Comment 17

[Martijn Wargers (dead)]

I see crashes happening in debug builds with layout/style/crashtests/512851-1.xhtml
It's not related to any file I changed here, but I suspect this might have something to do with layout/mathml/crashtests/400157.xhtml. That file is increasing docviewer.textZoom, but when it is done, that docviewer.textZoom isn't reset to 1.

Comment 18

[Martijn Wargers (dead)]

I filed bug 918834 for the crash, what I mentioned in comment 17 indeed seems to be the cause of this orange.

Comment 19

[Martijn Wargers (dead)]

Attachment: 917061.diff

This resets textZoom back to 1 when the test finishes.

Comment 20

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=4d43dfa2164d

Comment 21

[Martijn Wargers (dead)]

Comment on attachment 807790 [details] [diff] [review]
917061.diff

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

Sorry, this is probably a little bit overkill, but asking review again. Against the previous patch, I added this line:
+  docviewer.textZoom = 1;
To make sure that after the test, the textZoom is reset to the original state.

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #21)
> To make sure that after the test, the textZoom is reset to the original
> state.

To be really sure about this, I think we might need to explicitly cancel any pending "doe()" invocations, too.

(Note that there will always be a pending "doe" call, since it calls setTimeout to put itself back on the event queue.  It seems possible that we'd handle a pending doe() call during the time between reftest-wait-clearing and navigation to the next test, and that would be problematic because doe() adjusts the textZoom value.)

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #7)
> Ok, I'll do that. I guess I should remove the "setTimeout(function()
> {window.location.reload()}, 500);" call, because that can't work in a
> crashtest.

ALSO: In retrospect, I think we probably *do* need to remove the "reload()", too (as you suggested).

- If it actually were to fire, then it'd mean the crashtest never completes (because it'd never get to removing "reftest-wait"), which means we should remove it.
- If it doesn't fire, then it's useless, and we should remove it.

So either way, we should remove it.  (Sorry; I hadn't thought through that in Comment 8)

Comment 24

[Martijn Wargers (dead)]

Attachment: 917061.diff (for check-in)

Ok, I think this is what you want.

Comment 25

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=84f031582b74

Comment 26

[Daniel Holbert [:dholbert]]

Looks good, though if I run the crashtest locally with the patch applied, I get:
> JavaScript error: file:///scratch/work/builds/mozilla-inbound/mozilla/layout/mathml/crashtests/400157.xhtml, line 8: SpecialPowers is not defined

Do we need to include some script to get access to that?

Comment 27

[Daniel Holbert [:dholbert]]

(by "run the crashtest locally", I mean that I created a dummy manifest file "mine.list", containing _just_ "load 400157.xhtml", and I ran ./mach crashtest layout/mathml/crashtests/mine.list)

Comment 28

[Martijn Wargers (dead)]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> Looks good, though if I run the crashtest locally with the patch applied, I
> get:
> > JavaScript error: file:///scratch/work/builds/mozilla-inbound/mozilla/layout/mathml/crashtests/400157.xhtml, line 8: SpecialPowers is not defined
> 
> Do we need to include some script to get access to that?

No, I suspect this is a problem with the SpecialPowers extension, where the SpecialPowers object isn't attached to the first document that is loaded (after that, it works fine).
See http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowers.js?force=1#131

Could you edit your dummy manifest file to include one crashtest (that doesn't use SpecialPowers) that runs before 400157.xhtml and see if you still get this error?

Comment 29

[Martijn Wargers (dead)]

Never mind, I don't see the problem occuring.

Comment 30

[Martijn Wargers (dead)]

The issue described in comment 27 seems to be because of this:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftest.py#90
So the SpecialPowers extenstion isn't installed in combination with a custom manifest file.

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 810026 [details] [diff] [review]
917061.diff (for check-in)

Ah! That's annoying. Anyway, confirmed that the "not defined" issue goes away if I use crashtests.list instead of mine.list.

r=me

Comment 32

[Daniel Holbert [:dholbert]]

I filed bug 920706 on comment 27 / comment 30, BTW.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/baab6e4786ca

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/baab6e4786ca