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

NEW
Unassigned

Status

()

7 years ago
7 years ago

People

(Reporter: Felipe, Unassigned)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [e10s])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Created attachment 541502 [details] [diff] [review]
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)
(Reporter)

Comment 1

7 years ago
Created attachment 541504 [details] [diff] [review]
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
Attachment #541504 - Flags: review?(bzbarsky)
(Reporter)

Updated

7 years ago
Attachment #541504 - Flags: review?(hsivonen)
Comment on attachment 541504 [details] [diff] [review]
Test fix?

Seems to fix the failures with by about:blank patch, so r+!
Attachment #541504 - Flags: review?(hsivonen) → review+
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 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?
(Reporter)

Comment 5

7 years ago
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
Attachment #541504 - Flags: review?(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.
(Reporter)

Comment 7

7 years ago
Created attachment 541878 [details] [diff] [review]
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..
Attachment #541878 - Flags: review?(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.
(Reporter)

Comment 9

7 years ago
(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..
> 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....
(Reporter)

Comment 11

7 years ago
Created attachment 541890 [details] [diff] [review]
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
Attachment #541878 - Attachment is obsolete: true
Attachment #541890 - Flags: review?(bzbarsky)
Attachment #541878 - Flags: review?(bzbarsky)
(Reporter)

Updated

7 years ago
Attachment #541890 - Attachment is patch: true
Attachment #541890 - Attachment mime type: text/x-patch → text/plain
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.
Attachment #541890 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.