666753 - Adding a new progress listener to browser make two docShell tests to fail

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

I need to add a new progress listener to the frame script of the browser. But doing so is causing two docshell tests that involves nested iframes to fail, due to extra pagehide and unload events being dispatched, which those tests are not expecting to receive. They seem to be generated by about:blank loads

The attached patch is a minimal listener that triggers the problem.

test_bug301397.xul and test_bug364461.xul   (both mochitest-chrome)

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Test fix?

Forgot to mention that the problem is triggered by the access of aWebProgress.DOMWindow.

If this is indeed the current expected behavior, this patch fixes the tests by adding the new events to the list of expected events on those tests

Comment 2

[Henri Sivonen (:hsivonen)]

Comment on attachment 541504 [details] [diff] [review]
Test fix?

Seems to fix the failures with by about:blank patch, so r+!

Comment 3

[Henri Sivonen (:hsivonen)]

Comment on attachment 541504 [details] [diff] [review]
Test fix?

Hmm. Actually, this patch fixes only the first test for me, but the patch seems reasonable anyway.

Is e10s explicitly changing how about:blank works, by any chance?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 541504 [details] [diff] [review]
Test fix?

Could you please add comments for the new entries explaining what they're coming from?

Also, note that these tests are not Firefox-specific.  Will the code you're adding run in all Gecko-based browsers that run these tests?  Or just Firefox?

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 541504 [details] [diff] [review]
Test fix?

(In reply to comment #3)
> Comment on attachment 541504 [details] [diff] [review] [review]
>
> Is e10s explicitly changing how about:blank works, by any chance?

Nope, no changes there. The difference in the test seems to happen because the newly added listener touch DOMWindows during that test that wouldn't be touched before, and due to lazily creation they're now created and dispatch these events.

(In reply to comment #4)
> Could you please add comments for the new entries explaining what they're
> coming from?

Sure, I'll add a small comment and reference this bug #

> 
> Also, note that these tests are not Firefox-specific.  Will the code you're
> adding run in all Gecko-based browsers that run these tests?  Or just
> Firefox?

It will run everywhere, the code is being added to toolkit/content/widgets/browser.xml

Comment 6

[Boris Zbarsky [:bzbarsky]]

> It will run everywhere

In that case, I would prefer that we also change the test to always trigger those initial about:blank creations explicitly, so that it passes whether your other patch is applied or not.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Test fix

Added a comment and bug # to the added events

If possible I'd like to make the test changes in a follow-up because the test itself only issue commands written in docshell-helpers.js, so making modifications will possibly also change other tests that I don't need to handle now..

Comment 8

[Boris Zbarsky [:bzbarsky]]

> If possible I'd like to make the test changes in a follow-up.

The problem is that the moment you land this patch you will send non-Firefox trees orange.

> because the test itself only issue commands written in docshell-helpers.js

I'm suggesting you modify the pages the test loads, not the commands it issues...

For example, have the "iframe content #1" page touch the window inside the iframe right after the <iframe> is parsed and so forth.  Basically, make it so this test passes on Firefox without your other change.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #8)
> > If possible I'd like to make the test changes in a follow-up.
> 
> The problem is that the moment you land this patch you will send non-Firefox
> trees orange.

I don't understand how. Every tree will get both the code and test change, because the change is not Firefox specific.

> 
> > because the test itself only issue commands written in docshell-helpers.js
> 
> I'm suggesting you modify the pages the test loads, not the commands it
> issues...
> 
> For example, have the "iframe content #1" page touch the window inside the
> iframe right after the <iframe> is parsed and so forth.  Basically, make it
> so this test passes on Firefox without your other change.

ok let me try this..

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Every tree will get both the code and test change

Oh, I see.  I misunderstood what you were saying earlier.

I'd still prefer we fix this test to not depend on the browser UI....

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: Test fix

Ok, made the tests pass by themselves. I couldn't get it to work by waiting for load or DOMContentLoaded into the internal pages.. I never received the events that would cause new windows to be created. But it worked by adding the progress listeners to the test and waiting for the OnStateChange notifications

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 541890 [details] [diff] [review]
Test fix

I was thinking something much simpler, but this works too.  ;)

Please put the comments _before_ the lines they're documenting, though.  So for that first comment:

  // The following pagehide event is for the about:blank synthesized when the
  // sub-subframe's window is accessed at load start.

and similar for the others.  r=me with that.