Closed Bug 1411393 Opened 7 years ago Closed 7 years ago

marionette should not register beforeunload handlers

Categories

(Remote Protocol :: Marionette, defect, P2)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect

Tracking

(firefox-esr52 wontfix, firefox57 wontfix, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: erahm, Assigned: whimboo)

References

Details

(Whiteboard: [MemShrink:P1][17/11])

Attachments

(1 file)

Bug 1411393 - Marionette should not register listeners on the global window or document.
59 bytes, text/x-review-board-request
smaug
: review+
ato
: review+
Details 
Registering for `beforeunload` events effectively disables the bfcache which means we can't test for memory regressions in it.
What do you mean it “effectively disables the bfcache”?  Why and
where is beforeunload registered?
I see two uses of beforeunload in Marionette, one in
interaction.flushEventLoop [1] and one in the page load algorithm
[2].

The purpose of flushEventLoop should be self-explanatory, but I
think it is implemented the wrong way.  Instead of attaching a
beforeunload and requesting an animation frame, it should append a
custom event to the queue and wait for that to be processed.

For the wait-for-page-to-load case we need beforeunload to correctly
determine when the current document has successfully navigated.
whimboo can provide some more information on this, but I don’t
think there is any way of avoiding beforeunload here.

  [1] https://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/testing/marionette/interaction.js#309
  [2] https://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/testing/marionette/listener.js#142
Eric, how could it be determined if a page is denied from bfcache? If I would get this information I can have a look if the wait for page load feature is affected by that.

As Andreas pointed out we make use of it to indicate an upcoming page load activity. It has to be used because a normal `pagehide` event can have a not known delay, and as such we would have to wait extra time, which is not feasible.

If we cannot use `beforeunload` the only option as I can see would be to make use of the web progress listener again.

Olli, if you have a bit of time, maybe you could give some feedback? Thanks!
Flags: needinfo?(erahm)
Flags: needinfo?(bugs)
I don't understand the comment about pagehide having delay. Sure, it fires after beforeunload, but what is causing the issue there?
Flags: needinfo?(bugs)
I talked with Olli on IRC and he gave me some great feedback in how this can be solved. Basically what we should do is to NOT add our listeners for the bubbling phase, but the capturing phase, and that always on the tabchildglobal. We use the tabchildglobal already for most of the events except `beforeunload` and `unload`, because for those it was not working. But as Olli told me it's simply because those two events do not bubble. Which makes total sense now. As such we should also move those to the tabchildglobal and no-longer register listeners on the global window, or document.

Also all other listeners have to be updated too to work for the capturing phase.
I changed all the `addEventListener` and `removeEventListener` invocations to be called on the tabchildglobal, and using the capturing phase. All the tests work fine, but I see a misbehavior of Firefox in the following test:

https://dxr.mozilla.org/mozilla-central/rev/967c95cee709756596860ed2a3e6ac06ea3a053f/testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py#238-245

The test is doing the following:

* Navigating to https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/www/navigation_pushstate.html

* Click the forward link (no page load activity happens here because of pushstate)

* Ensures that https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/www/navigation_pushstate_target.html has been loaded, and that the element with the id=target is not present

* Click the back button (popstate event)

* Ensures that https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/www/navigation_pushstate.html is displayed

* Click the forward button (popstate event)

* Ensures that https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/www/navigation_pushstate_target.html has been loaded, and that the element with the id=target is not present

* Navigating to https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/www/test.html (DOMContentLoaded, and pageshow events)

* Click the back button (no DOMContentLoaded event, but only pageshow event?)

* Ensures that https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/www/navigation_pushstate_target.html has been loaded

* Check that the element with the id=target is present

Hereby the last check fails:

* The document of the loaded page is not from `navigation_pushstate_target.html` but `marionette_harness/www/navigation_pushstate.html`, and as result of it the element with the id target does not exist.

This behavior is only visible when I make the relevant changes to `addEventListener` and `removeEventListener`. It works fine otherwise.

Here an excerpt of the events:

goForward() before full page load:
----------------------------------
1508932248312	Marionette	TRACE	3 -> [0,27,"goForward",{}]
1508932248319	Marionette	DEBUG	Received DOM event "popstate" for "http://127.0.0.1:61204/navigation_pushstate_target.html"
1508932248329	Marionette	TRACE	3 <- [1,27,null,{}]
1508932248332	Marionette	TRACE	3 -> [0,28,"getCurrentUrl",{}]
1508932248333	Marionette	TRACE	3 <- [1,28,null,{"value":"http://127.0.0.1:61204/navigation_pushstate_target.html"}]
1508932248337	Marionette	TRACE	3 -> [0,29,"findElement",{"using":"id","value":"target"}]
1508932248342	Marionette	TRACE	3 <- [1,29,{"error":"no such element","message":"Unable to locate element: target","stacktrace":"WebDriverError@chrome://marionette/content/error.js:172:5\nNoSuchElementError@chrome://marionette/content/error.js:400:5\neleme ... ync*dispatch/</req<@chrome://marionette/content/listener.js:488:14\ndispatch/<@chrome://marionette/content/listener.js:483:15\n"},null]

navigation to test.hml:
-----------------------
1508932248344	Marionette	TRACE	3 -> [0,30,"get",{"url":"http://127.0.0.1:61204/test.html"}]
1508932248346	Marionette	DEBUG	Received DOM event "beforeunload" for "http://127.0.0.1:61204/navigation_pushstate_target.html"
1508932248370	Marionette	DEBUG	Received DOM event "pagehide" for "http://127.0.0.1:61204/navigation_pushstate_target.html"
++DOMWINDOW == 12 (0x11acd3000) [pid = 12524] [serial = 12] [outer = 0x11be7a800]
1508932248390	Marionette	DEBUG	Received DOM event "DOMContentLoaded" for "http://127.0.0.1:61204/test.html"
1508932248406	Marionette	DEBUG	Received DOM event "pageshow" for "http://127.0.0.1:61204/test.html"
1508932248410	Marionette	TRACE	3 <- [1,30,null,{}]
1508932248428	Marionette	TRACE	3 -> [0,31,"getCurrentUrl",{}]
1508932248430	Marionette	TRACE	3 <- [1,31,null,{"value":"http://127.0.0.1:61204/test.html"}]

goBack():
---------
1508932248436	Marionette	TRACE	3 -> [0,32,"goBack",{}]
[Child 12524, Main Thread] WARNING: Cannot know response Content-Length due to presence of Content-Encoding or Transfer-Encoding headers.: file /builds/worker/workspace/build/src/dom/fetch/FetchDriver.cpp, line 549
1508932248460	Marionette	DEBUG	Received DOM event "beforeunload" for "http://127.0.0.1:61204/test.html"
1508932248462	Marionette	DEBUG	Received DOM event "pagehide" for "http://127.0.0.1:61204/test.html"
1508932248467	Marionette	DEBUG	Received DOM event "pageshow" for "http://127.0.0.1:61204/navigation_pushstate_target.html"
1508932248504	Marionette	TRACE	3 <- [1,32,null,{}]
1508932248516	Marionette	TRACE	3 -> [0,33,"getCurrentUrl",{}]
1508932248517	Marionette	TRACE	3 <- [1,33,null,{"value":"http://127.0.0.1:61204/navigation_pushstate_target.html"}]
1508932248520	Marionette	TRACE	3 -> [0,34,"findElement",{"using":"id","value":"target"}]
1508932248525	Marionette	TRACE	3 <- [1,34,{"error":"no such element","message":"Unable to locate element: target","stacktrace":"WebDriverError@chrome://marionette/content/error.js:172:5\nNoSuchElementError@chrome://marionette/content/error.js:400:5\neleme ... ync*dispatch/</req<@chrome://marionette/content/listener.js:488:14\ndispatch/<@chrome://marionette/content/listener.js:483:15\n"},null]

Olli, I'm not sure why this changed behavior is visible when I flip the listeners to capture mode. Maybe I also understood something wrong at https://developer.mozilla.org/en-US/docs/Web/API/History_API and the test is wrong?
Flags: needinfo?(bugs)
Does the web page have listeners too? Those get run after capture phase listeners in tabchildglobal.
You may also want to try system group listener in capture phase.
http://searchfox.org/mozilla-central/source/dom/webidl/EventTarget.webidl#18
System group listeners are called after normal (capturing/bubbling) listeners
Flags: needinfo?(bugs)
I'm assuming Olli has you covered, let me know if you need anything else.
Flags: needinfo?(erahm)
Blocks: 1411264
Whiteboard: [MemShrink] → [MemShrink:P1]
Blocks: 1394351
(In reply to Olli Pettay [:smaug] from comment #7)
> Does the web page have listeners too? Those get run after capture phase
> listeners in tabchildglobal.

No, only the element which receives the click has an onclick attribute set:

https://dxr.mozilla.org/mozilla-central/rev/b5a3b8ef6902998507fc881b6d628b055457fe31/testing/marionette/harness/marionette_harness/www/navigation_pushstate.html#18

The target page has no listeners at all:

https://dxr.mozilla.org/mozilla-central/rev/b5a3b8ef6902998507fc881b6d628b055457fe31/testing/marionette/harness/marionette_harness/www/navigation_pushstate_target.html

But I also see this when performing the clicks manually after Marionette started Firefox. Could that mean it's a possible bug in Firefox?

> You may also want to try system group listener in capture phase.
> http://searchfox.org/mozilla-central/source/dom/webidl/EventTarget.webidl#18
> System group listeners are called after normal (capturing/bubbling) listeners

I will try this next. Thanks.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: -- → P2
Flags: needinfo?(bugs)
(In reply to Henrik Skupin (:whimboo) from comment #9)
> > You may also want to try system group listener in capture phase.
> > http://searchfox.org/mozilla-central/source/dom/webidl/EventTarget.webidl#18
> > System group listeners are called after normal (capturing/bubbling) listeners
> 
> I will try this next. Thanks.

With a latest build of Firefox I can no longer reproduce the problem as mentioned earlier when using the capturing phase. Also the system group listeners work like expected. Maybe there was really a bug which got recently fixed?

Given that we want to get informed as early as possible I would stick with the capturing phase. The system group listeners might have a delay given that normal listeners are getting called first.
There hasn't been relevant changes to the Gecko's DOM event dispatch.
Flags: needinfo?(bugs)
Eric, can you please try one of the try builds and give me feedback if those changes fix your issue? I just submitted, so it might take a little bit for the builds to appear.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=586ed74fa63b
Flags: needinfo?(erahm)
(In reply to Olli Pettay [:smaug] from comment #11)
> There hasn't been relevant changes to the Gecko's DOM event dispatch.

Strange then. Not sure what caused those issues for me then. Anyway, it's looking fine now. Lets wait for the try results...
Attachment #8925007 - Flags: review?(bugs)
Attachment #8925007 - Flags: review?(ato)
Comment on attachment 8925007 [details]
Bug 1411393 - Marionette should not register listeners on the global window or document.

https://reviewboard.mozilla.org/r/196256/#review201478

I wonder if we would also have to change the registration of beforeunload in interaction.flushEventLoop().
(In reply to Henrik Skupin (:whimboo) from comment #15)

> Comment on attachment 8925007 [details] Bug 1411393 - Marionette should not
> register listeners on the global window or document.
>
> https://reviewboard.mozilla.org/r/196256/#review201478
>
> I wonder if we would also have to change the registration of
> beforeunload in interaction.flushEventLoop().

I’m happy to fix interaction.flushEventLoop() to do what I
think it should do (explained above in comment #2).  Filed
https://bugzil.la/1414329 to follow up on this.
Comment on attachment 8925007 [details]
Bug 1411393 - Marionette should not register listeners on the global window or document.

https://reviewboard.mozilla.org/r/196258/#review201538
Attachment #8925007 - Flags: review?(ato) → review+
Whelp on the plus side it clearly worked:

> │  ├───14.61 MB (06.71%) -- top(http://localhost:8097/page_load_test/tp5n/homeway.com.cn/www.hexun.com/index.html, id=NNN)
> │  │   ├──13.10 MB (06.01%) -- **cached**
> │  │   │  ├───9.86 MB (04.52%) -- window(http://localhost:8067/page_load_test/tp5n/bild.de/www.bild.de/index.html)


On the down side we regressed 219 MiB on our tabs open measurement [1]. I knew we'd regress but this seems pretty bad. Olli does this make sense? I feel like there might have been a regression over the past 7 months.

[1] https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=586ed74fa63bd50ed59e3455e55d0989397408fa&originalSignature=a630e0d4f7000610ca57d0f8da52b55d117632a9&newSignature=a630e0d4f7000610ca57d0f8da52b55d117632a9&framework=4&selectedTimeRange=172800
Flags: needinfo?(erahm) → needinfo?(bugs)
I’ve submitted a patch to https://bugzil.la/1414329 which fixes
the last usage of beforeunload in Marionette.  It would normally
only come into effect when clicking an element.
Depends on: 1414329
I don't know what tabs open measurement measures. But if it loads several pages to a tab, then using more memory when bfcache is enabled makes of course sense.
We do keep bfcache quite a while alive
http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/docshell/shistory/nsSHistory.cpp#44
Flags: needinfo?(bugs)
Comment on attachment 8925007 [details]
Bug 1411393 - Marionette should not register listeners on the global window or document.

https://reviewboard.mozilla.org/r/196258/#review201674

::: testing/marionette/listener.js:142
(Diff revision 1)
> -      addEventListener("pagehide", this, false);
> -      addEventListener("popstate", this, false);
> +      addEventListener("pagehide", this, true);
> +      addEventListener("popstate", this, true);
>  
>        // The events can only be received when the event listeners are
>        // added to the currently selected frame.
> -      curContainer.frame.addEventListener("beforeunload", this);
> +      addEventListener("beforeunload", this, true);

So the listener does check that the events are coming from the right frame? Looks like so.
But please fix the comment then above these addEventListener calls.

::: testing/marionette/listener.js:191
(Diff revision 1)
> -    removeEventListener("DOMContentLoaded", this);
> -    removeEventListener("pageshow", this);
> +    removeEventListener("DOMContentLoaded", this, true);
> +    removeEventListener("pageshow", this, true);
>  
>      // If the original content window, where the navigation was triggered,
>      // doesn't exist anymore, exceptions can be silently ignored.
>      try {

Why you still need this try-catch around removeEventListener?
Attachment #8925007 - Flags: review?(bugs) → review+
Comment on attachment 8925007 [details]
Bug 1411393 - Marionette should not register listeners on the global window or document.

https://reviewboard.mozilla.org/r/196258/#review201674

> So the listener does check that the events are coming from the right frame? Looks like so.
> But please fix the comment then above these addEventListener calls.

Oh, right. There is indeed no special-casing necessary anymore. I will fix it with the next update.

> Why you still need this try-catch around removeEventListener?

Same as above. Thanks for noticing that! It will be removed.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/59856e397ead
Marionette should not register listeners on the global window or document. r=ato,smaug
Whiteboard: [MemShrink:P1] → [MemShrink:P1][17/11]
https://hg.mozilla.org/mozilla-central/rev/59856e397ead
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
The `beforeunload` listener has been added via bug 1357634 for Firefox 54. And as such it is present for the last Firefox releases.

I wonder about the impact this causes for testers when the websites under test are not getting served as expected. Means no bfcache is used. If that is a problem we should consider to uplift this patch to Firefox 57 as long as it hasn't been released yet.

On the other side no-one who is using Firefox 55, 56, or 57 beta for Selenium tests actually complained about it yet. Maybe we should just let it ride the train to reduce any kind of unexpected side-effect.

David, can you please weight in here?
Blocks: 1357634
Flags: needinfo?(dburns)
Depends on: 1415125
Depends on: 1415164
Let's just let it ride the trains
Flags: needinfo?(dburns)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: