Closed Bug 546648 Opened 15 years ago Closed 15 years ago

[HTML5] toolkit/content/tests/widgets/test_mousecapture.xul fails

Categories

(Core :: DOM: HTML Parser, defect, P1)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(2 files, 1 obsolete file)

Add a > to complete a tag in data URL
(deleted), patch
enndeakin
: review+
Details | Diff | Splinter Review
Wait for the load event when opening new windows
(deleted), patch
enndeakin
: review+
Details | Diff | Splinter Review 
148939 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_mousecapture.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - aTarget is null at http://localhost:8888/tests/SimpleTest/EventUtils.js:213
Attached patch Add a > to complete a tag in data URL (deleted) — — Splinter Review 
Note: The HTML5 parsing algorithm ignores an incomplete tag token at EOF as a supposed security measure.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #427763 - Flags: review?(enndeakin)
Summary: [HTML5] toolkit/content/tests/widgets/test_mousecapture.xul fails → [HTML5][Patch] toolkit/content/tests/widgets/test_mousecapture.xul fails
Attachment #427763 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/f4e40572b572
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Still fails.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: [HTML5][Patch] toolkit/content/tests/widgets/test_mousecapture.xul fails → [HTML5] toolkit/content/tests/widgets/test_mousecapture.xul fails
Works for me locally, though.
Summary: [HTML5] toolkit/content/tests/widgets/test_mousecapture.xul fails → [HTML5][Patch] toolkit/content/tests/widgets/test_mousecapture.xul fails
waitForFocus already does a wait for the load event. Is this not working as expected?
(In reply to comment #7)
> waitForFocus already does a wait for the load event. Is this not working as
> expected?

It appears that it isn't. Without the patch the test fails (on try but not locally). With the patch it succeeds.
Untested wild guess written down as a note to self: Could it be that waitForFocus sees an nsDocShell::EnsureContentViewer-created temporary about:blank document whose readyState is "complete"?
I thought of that possibility. Perhaps waitForFocus should wait for another load event if the current location is 'about:blank' and is the readyState is 'complete'.
I tried making EnsureContentViewer set the readyState of the about:blank doc to loading, but that didn't help.
Let's see what this does on the tryserver.
Comment on attachment 432514 [details] [diff] [review]
Try patching the test harness to check window.location vs. document URI

Didn't work out.
Attachment #432514 - Attachment is obsolete: true
Can we tell if the temporary about:blank is present and wait for a load anyway?

I think this issue may be affecting or has affected other tests as well.
Attachment #431046 - Flags: review?(enndeakin)
(In reply to comment #14)
> Can we tell if the temporary about:blank is present and wait for a load anyway?

I can't think of a non-brittle way to detect if about:blank is temporary or not. (I *think* there's a brittle way relying on another bug: IIRC, the EnsureContentViewer about:blank DOM and the stream-parsed about:blank DOM aren't identical. But let's not go there.)

> I think this issue may be affecting or has affected other tests as well.

I have now tried checking if document.documentURI of the doc is about:blank and I've tried making the docshell mark the temporary about:blank as having readyState "loading". In both cases, other tests fall over. (I've also tried comparing targetWindow.location.href with targetWindow.document.documentURI, but it appears that's not a robust way of detecting temporary about:blank after all.)

It seems to me that other tests are depending on waitForFocus not actually waiting.

What I don't understand is why waitForFocus tries to be smart about avoiding the load event handler is some cases. Wouldn't it be simpler to have a mechanism that always waits for the load event and leave it up to the caller of the method not to use it in cases where a load event isn't expected?

Anyway, I really wish the enablement of the HTML5 parser didn't need to block on changing waitForFocus across all tests.

I suggest the following:
 * Landing the patch for this test case.
 * Introducing a new waitForFocus-like helper method that always installs a load event handler.
 * Using the new helper method in new tests.
 * Transitioning old tests to using the new method when old tests are caught as random orange.

Fixing bug 543435 *might* lead to a more proper long-term fix, but bug 543435 is such a can of worms in itself as far as test case compat goes that it was taken off the critical path of the HTML5 parser enablement.
(In reply to comment #15)

> 
> I have now tried checking if document.documentURI of the doc is
> about:blank and I've tried making the docshell mark the temporary
> about:blank as having readyState "loading". In both cases, other tests
> fall over.

How many tests? Maybe those need fixing too?


> It seems to me that other tests are depending on waitForFocus not actually
> waiting.
> 
> What I don't understand is why waitForFocus tries to be smart about avoiding
> the load event handler is some cases. Wouldn't it be simpler to have a
> mechanism that always waits for the load event and leave it up to the caller of
> the method not to use it in cases where a load event isn't expected?

The caller doesn't know what events have been fired or which ones are still to come. The order of load and focus events is not guaranteed. The point of waitForFocus is to figure all that out for you, so that the test can get on with testing what it needs to test.


> I suggest the following:
>  * Landing the patch for this test case.
>  * Introducing a new waitForFocus-like helper method that always installs a
> load event handler.
>  * Using the new helper method in new tests.
>  * Transitioning old tests to using the new method when old tests are caught as
> random orange.

If all new tests could use this, that would imply that you're guaranteeing that the load event hasn't fired when the waitForFocus call is made.
Is the implication here that tests should always be calling waitForFocus before the load event fires, and that if waitForFocus is changed to always wait for a load event, the tests that fail should be fixed?
(In reply to comment #17)
> Is the implication here that tests should always be calling waitForFocus before
> the load event fires, and that if waitForFocus is changed to always wait for a
> load event, the tests that fail should be fixed?

Yes, that's what I'm suggesting.

For fixing the failing events, there could be a variant that doesn't wait for the load event, ever. I think it would be less fragile for the test author to decide whether the load event needs waiting for and not have the library try to detect if it needs to wait for the load event.
(In reply to comment #16)
> How many tests? 

I'll rerun the tests.

> Maybe those need fixing too?

Sure. The thing is that this bug is the second to last test failure preventing the HTML5 parser from being turned on by default. If I need to fix a bunch of other latent potential random oranges as a prerequisite for turning the HTML5 on by default, during that time more tests that haven't been tried with the HTML5 parser get pushed to m-c and round and round we go. That's why I'm so keen to paper this one over even though I know the patch doesn't go to the heart of the problem.
(In reply to comment #19)
> (In reply to comment #16)
> > How many tests? 
> 
> I'll rerun the tests.

Both scenarios cause timeouts, so it's unknown how many tests fail.
Why doesn't
    if (!SimpleTest.waitForFocus_loaded) {
        ok(true, "must wait for load");
        targetWindow.addEventListener("load", waitForEvent, false);
    }
in SimpleTest.js return early after adding the event listener?
More data: When I run this locally and the test *succeeds*, waitForFocus makes its decision not to attach an 'load' handler by seeing an about:blank document in readyState "complete".
More data (the most surprising finding to date): The about:blank doc mentioned in the previous comment is *not* generated by EnsureContentViewer but parsed from an about: channel (by the old parser). (No wonder tweaking the readyState in EnsureContentViewer didn't help.)
(In reply to comment #23)
> More data (the most surprising finding to date): The about:blank doc mentioned
> in the previous comment is *not* generated by EnsureContentViewer but parsed
> from an about: channel (by the old parser).

Verified on the tryserver that this is also the situation on the tryserver (on Mac and Windows; Linux didn't fail in this test run).
Compared the logs with the old parser and with the HTML5 parser:
Right before the failure, these things happen in both cases:
1) waitForFocus sees stream-parsed about:blank in readyState complete.
2) waitForFocus decides to wait for focus (but not to wait for load)

Then with the old parser, the following happens:
3) MozAfterPaint event received (apparently, the data: URL got parsed synchronously)
4) Proceed to running test callback.

With the HTML5 parser, the following happens instead:
3) Proceed to running the test callback.
4) Failure because the data: URL was only scheduled to get parsed but hasn't completed yet.

From this data, I conclude that indeed the HTML5 parser is working as designed but waitForFocus isn't. That is, at step #2 waitForFocus() should decide to wait for a load event.

Unfortunately, I am not able to suggest how waitForFocus() could make the right decision. That is, I'm unaware of an API for querying if a window is about to navigate away from its currently-loaded document.
Summary: [HTML5][Patch] toolkit/content/tests/widgets/test_mousecapture.xul fails → [HTML5] toolkit/content/tests/widgets/test_mousecapture.xul fails
(In reply to comment #25)
> Compared the logs with the old parser and with the HTML5 parser:
> Right before the failure, these things happen in both cases:
> 1) waitForFocus sees stream-parsed about:blank in readyState complete.

No, this is the bug. The test never asks for about:blank to be loaded, nor does it ever ask for a blank window to be opened. That some internal behaviour is exposed to the page or script is a serious problem that numerous tests have had to workaround and I have wasted significant amounts of time having to deal with this.

> With the HTML5 parser, the following happens instead:
> 3) Proceed to running the test callback.
> 4) Failure because the data: URL was only scheduled to get parsed but hasn't
> completed yet.

If I asked for a url to be loaded, I expect it to not indicate that it is loaded until that has happened. If 'readyState' isn't the right indicator for that then that's fine, but I know of no other means of knowing when the 'url is really ready' state occurs.
More data: Having a stream-parsed about:blank right after window.open() is specific to doing window.open() from XUL or chrome. Doing window.open() from Web content behaves like an iframe in Web content (docshell-created about:blank with readyState uninitialized). Demo:
http://hsivonen.iki.fi/test/moz/window-open-initial.html
(In reply to comment #27)
> http://hsivonen.iki.fi/test/moz/window-open-initial.html

Additional data of how different engines behave with that demo:
Gecko: about:blank, readyState uninitialized
WebKit: about:blank, readyState complete (like Gecko in the chrome/XUL case!)
Presto: about:blank, readyState interactive
Trident (IE8 mode): URL being navigated to, readyState loading

readyState has four possible states, and testing four browser engines covers them all! Yay for interop.
(In reply to comment #28)
> Additional data of how different engines behave with that demo:
> Gecko: about:blank, readyState uninitialized
> WebKit: about:blank, readyState complete (like Gecko in the chrome/XUL case!)
> Presto: about:blank, readyState interactive
> Trident (IE8 mode): URL being navigated to, readyState loading

The only case we care about is:
  URL being navigated to, readyState complete
Otherwise, a load event should be waited for.
(In reply to comment #29)
> (In reply to comment #28)
> > Additional data of how different engines behave with that demo:
> > Gecko: about:blank, readyState uninitialized
> > WebKit: about:blank, readyState complete (like Gecko in the chrome/XUL case!)
> > Presto: about:blank, readyState interactive
> > Trident (IE8 mode): URL being navigated to, readyState loading
> 
> The only case we care about is:
>   URL being navigated to, readyState complete
> Otherwise, a load event should be waited for.

The problem is waitForFocus doesn't know that the URL being navigated to is, so it can't compare it against targetWindow.location.href.

One way to work around this would be introducing a library method SimpleTest.openWindowAndWaitForFocus(url, callback, params) that'd then call window.open() and would know what URL it's trying to load.

(In reply to comment #27)
> More data: Having a stream-parsed about:blank right after window.open() is
> specific to doing window.open() from XUL or chrome. Doing window.open() from
> Web content behaves like an iframe in Web content (docshell-created about:blank
> with readyState uninitialized). Demo:
> http://hsivonen.iki.fi/test/moz/window-open-initial.html

It seems to me that the difference between content and chrome window.open and how the initial about:blank ends up in there was introduced in bug 158370.
(In reply to comment #30)
> The problem is waitForFocus doesn't know that the URL being navigated to is, so
> it can't compare it against targetWindow.location.href.

Isn't the URL being navigated to any URL that is not 'about:blank'?

The rare case where the 'about:blank' is actually desired is a special case that can be handled separately if and when it is needed.
(In reply to comment #31)
> (In reply to comment #30)
> > The problem is waitForFocus doesn't know that the URL being navigated to is, so
> > it can't compare it against targetWindow.location.href.
> 
> Isn't the URL being navigated to any URL that is not 'about:blank'?

Except when about:blank is the desired destination as you note below.

> The rare case where the 'about:blank' is actually desired is a special case
> that can be handled separately if and when it is needed.

I don't understand why it's better to have separate things for test writers to do for non-about:blank vs. about:blank compared to having separate things to do when a load event listener is needed vs. when a load event listener is not needed.
(In reply to comment #32)
> I don't understand why it's better to have separate things for test writers to
> do for non-about:blank vs. about:blank

I can't imagine that many tests would need to intentionally load an 'about:blank' document and need it to be focused. Perhaps browser chrome tests opening a new tab. An extra 'isAboutBlank' argument could easily be added to waitForFocus though for just that occasion.


> when a load event listener is needed vs. when a load event listener is not
> needed.

As I've said, the test writer doesn't care what load or focus events are needed or not, nor should they care. We want to make it easy to write tests and whether a page is loaded, whether some bogus about:blank exists and whether the right window is focused has nothing to do with what the test is trying to test.

The difference is that the test author easily knows whether they are trying to load a blank document or not.
(In reply to comment #33)
> (In reply to comment #32)
> > I don't understand why it's better to have separate things for test writers to
> > do for non-about:blank vs. about:blank
> 
> I can't imagine that many tests would need to intentionally load an
> 'about:blank' document and need it to be focused. Perhaps browser chrome tests
> opening a new tab. An extra 'isAboutBlank' argument could easily be added to
> waitForFocus though for just that occasion.

OK. I'm not pursuing this way of dealing with this test failure.

> > when a load event listener is needed vs. when a load event listener is not
> > needed.
> 
> As I've said, the test writer doesn't care what load or focus events are needed
> or not, nor should they care. We want to make it easy to write tests and
> whether a page is loaded, whether some bogus about:blank exists and whether the
> right window is focused has nothing to do with what the test is trying to test.
> 
> The difference is that the test author easily knows whether they are trying to
> load a blank document or not.

So far, it seems that this is not the case. That is, the tests that I've adjusted so far didn't seem to intend to navigate to about:blank.
> So far, it seems that this is not the case. That is, the tests that I've
> adjusted so far didn't seem to intend to navigate to about:blank.

Which tests? Why did they need to be changed?
(In reply to comment #35)
> > So far, it seems that this is not the case. That is, the tests that I've
> > adjusted so far didn't seem to intend to navigate to about:blank.
> 
> Which tests?

chrome://mochikit/content/browser/browser/base/content/test/browser_NetworkPrioritizer.js
chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_524745.js 

> Why did they need to be changed?

Checking for about:blank in waitForFocus made then time out. It's not obvious why and how exactly they are depending on the pre-existing waitForFocus behavior with about:blank.
Once I fixed those, the following time out:
1) /tests/MochiKit_Unit_Tests/test_MochiKit-Async.html
2) chrome://mochikit/content/chrome/browser/components/feeds/test/chrome/test_423060.xul
3) (unknown test timed out)
4) chrome://mochikit/content/a11y/accessible/actions/test_anchors.html
5) (something after e:\builds\moz2_slave\win32-unit\mozilla\objdir\_tests\xpcshell\test_necko\test\test_name_scheme.js)
> No, this is the bug. The test never asks for about:blank to be loaded

All windows contain about:blank until something else is loaded, as far as script is concerned.  This is fundamental behavior the web depends on.

> If 'readyState' isn't the right indicator for that then that's fine

It's not the right indicator.  readyState is the indicator for the loading state of a _document_.  If your requested load hasn't gotten far enough to have created a new document for itself yet, then you're asking about the loading state of whatever was there before your load started, since you're examining the document that was already there before you started your load.

> but I know of no other means of knowing when the 'url is
> really ready' state occurs.

You could use the same thing onload firing uses: empty loadgroup.  If you need to worry about subframes, then you need an empty loadgroup _and_ a complete readyState.
Comment on attachment 431046 [details] [diff] [review]
Wait for the load event when opening new windows

OK, let's just fix the test then, but please file a different bug so that we can fix up waitForFocus as needed.
Attachment #431046 - Flags: review+
Thanks!

Filed bug 554873. Pushed the patch:
http://hg.mozilla.org/mozilla-central/rev/aea3ee1a9baf
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: