Closed Bug 917061 Opened 6 years ago Closed 6 years ago

Fix the last 2 crashtests that use enablePrivilege

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

Details

Attachments

(1 file, 6 obsolete files)

Attached patch crashtest.diff (obsolete) — Splinter Review
I haven't tested yet if this works, ./mach crashtest doesn't seem to work here, locally.
It turns out that accessible/tests/crashtests/471493.xul doesn't run under crashtests.
Not sure how they are run on tbpl and tryserver.
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
(So: if you want to test the /accessible patch, you'll need to add that line back to testing/crashtest/crashtests.list)
Attached patch 917061.diff (obsolete) — Splinter Review
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
Attachment #805693 - Attachment is obsolete: true
Attachment #806322 - Flags: review?(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.
Attachment #806322 - Flags: review?(dholbert) → review+
(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.
(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.)
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.
Attached patch 917061.diff (obsolete) — Splinter Review
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.
Attachment #806322 - Attachment is obsolete: true
Attachment #806995 - Flags: review?(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
Attachment #806995 - Flags: review?(dholbert) → review+
(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.
Attached patch 917061.diff (obsolete) — Splinter Review
Ok, done your suggestion in comment 11. Thanks for you help.
Attachment #806995 - Attachment is obsolete: true
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)
Attached patch 917061.diff (obsolete) — Splinter Review
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.
Attachment #807119 - Attachment is obsolete: true
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.
I filed bug 918834 for the crash, what I mentioned in comment 17 indeed seems to be the cause of this orange.
Attached patch 917061.diff (obsolete) — Splinter Review
This resets textZoom back to 1 when the test finishes.
Attachment #807680 - Attachment is obsolete: true
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.
Attachment #807790 - Flags: review?(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.)
(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)
Ok, I think this is what you want.
Attachment #807790 - Attachment is obsolete: true
Attachment #807790 - Flags: review?(dholbert)
Attachment #810026 - Flags: review?(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?
(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)
(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?
Never mind, I don't see the problem occuring.
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 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
Attachment #810026 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Attachment #810026 - Attachment description: 917061.diff → 917061.diff (for check-in)
https://hg.mozilla.org/integration/fx-team/rev/baab6e4786ca
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/baab6e4786ca
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.