Add crashtest to be sure we fire pagehide event in documents with sync XHR during pageload

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla68
Points:
---

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 months ago

My initial patch in bug 1535388 would've prevented us from firing pageshow if there was a sync XHR during pageload.

It nearly passed on Try (aside from one arcane failure in test_disableScript.xul), so let's add some crashtests that more directly test that pageshow is indeed fired which would've caught my mistake.

Updated

3 months ago
Component: DOM: Events → DOM: Core & HTML
Priority: -- → P5

Updated

3 months ago
Priority: P5 → P2
Attachment #9052438 - Attachment description: Bug 1537588: Add crashtests to verify that pagehide and pageshow are fired, even if there's a sync XHR during pageload. r?bzbarsky → Bug 1537588: Add crashtest to verify that pageshow is fired, even if there's a sync XHR during pageload. r?bzbarsky
Assignee

Comment 2

3 months ago

My "pagehide" variant doesn't seem to pass for some reason, actually, so I'll just add the pageshow one for now. That one reliably passes, locally, with a mozilla-central debug build.

(Updated patch)

Assignee

Comment 4

3 months ago

Apparently this event isn't reliably fired in this scenario, even in normal mozilla-central. My crashtest was passing reliably yesterday, locally, and it passes on Android (it's shown as passing in the log for Android 4.3 opt C1 and debug C1 in my try run above) -- but it failed on all other Try platforms (specifically, it never fired pageshow, so it timed out).

I tried to rewrite the test with the sync XHR + navigation happening in an iframe, but that doesn't seem to help -- that still makes pageshow fire quite rarely (with "###!!! ASSERTION: Destroying a currently-showing document" being spammed most of the time).

I'm experimenting with an iframe-based pagehide test now, though, which seems more reliable locally (despite comment 2).
If that works, I might just settle for that at the moment, and hopefully we can extend it to cover pageshow as well once bug 1535388 or another followup is addressed.

Attachment #9052438 - Attachment is obsolete: true
Assignee

Comment 6

3 months ago

Here's a Try run with a slightly older but functionally-equivalent version of this crashtest:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0320684141902b2311e41e5747aef8227d79aa29

(That try run is layered on top of bug 1537588's patch -- without that patch, this crashtest causes an assertion-failure "Destroying a currently-showing document" as noted in comment 4, so we need that patch before we can land this crashtest.)

Assignee

Updated

3 months ago
Summary: Add crashtest to be sure we fire pagehide & pageshow events in documents with sync XHR during pageload → Add crashtest to be sure we fire pagehide event in documents with sync XHR during pageload

Comment 7

3 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae7a26049a6c
Add crashtest to verify that pagehide is fired, even if there's a sync XHR during pageload. r=bzbarsky

Comment 8

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
https://hg.mozilla.org/projects/ash/rev/ae7a26049a6c8e46b5a80c0cdbe9b7125219e1c9
Bug 1537588: Add crashtest to verify that pagehide is fired, even if there's a sync XHR during pageload. r=bzbarsky
You need to log in before you can comment on or make changes to this bug.