1629201 - Figure out how to unify code for firing load and error events to <iframe> elements and OOP children

Status: NEW

(Core :: DOM: Navigation, task, P3)

Description

[Matt Woodrow (:mattwoodrow)]

We currently have a lot of code for firing load/error events spread across a lot of different areas that makes it very hard to reason about.

• For firing load to the <iframe> element:

nsDocumentViewer::LoadComplete fires a load event for the inner document if the load was a success. We have a TODO here for also firing an error event, but note that this would only run if the load got far enough to create a document viewer.

nsGlobalWindowInner::PostHandleEvent catches the load event, and forwards it to the embedder with FireFrameLoadEvent. If we are an out of process iframe, then this also sends it across IPC to the embedder, and unblocks the load event of the outer document.

• For firing error to the <iframe> element:

This is fired by nsFrameLoader::FireErrorEvent. It's only called for errors that we synchronously detect, so doesn't ever fire for OOP iframes, loads that go through Document::InitializeFrameLoader, or for errors returned asynchronously from the channel within docshell.

It's hard to know exactly which set of possible errors this is, and how it's being changed by fission moving more <iframe>s OOP.

• Unblocking the load event (for the Document) when an OOP iframe finishes loading:

When the inner Document finishes loading, we call BrowserChild::SendMaybeFireEmbedderLoadEvents to notify the outer Document. When this happens from the nsGlobalWindowInner callsite, it also acts as the way we request the load event to be fired on the <iframe> element.

There are callers to this function spread across nsDocShell, nsDocLoader and nsGlobalWindowInner. These are trying to catch every possible case in which loading might be done, and frequently we hit multiple of them for a single load (which is safe, but a bit weird).

It seems like we'd ideally have a single owner for knowing when loading is complete, and then we can have clear decision making on which events to fire (and for which error code types).

I suspect we're not firing error anywhere near as much as we should (and this is changing with fission), but I think we need to clean this up before we can make much progress on figuring out exactly which failure types need an error event.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1629201 - Add option to MaybeFireEmbedderLoadEvents to also fire the error event to the embedder element. r?nika

Depends on D70584

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1629201 - Fire onerror event for CONTENT_BLOCKED in EndPageLoad. r?nika,ckerschb

Depends on D70585

Comment 3

[Tom S [:evilpie]]

Do note that it seems like Chrome for example often fires load events instead of error events, even when an error occurred: i.e. Bug 1601887, Bug 1418975, Bug 1552504.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Yeah, need to be very careful when changing load and error event dispatching here.

Comment 5

[Phabricator Automation]

Comment on attachment 9139899 [details]
Bug 1629201 - Add option to MaybeFireEmbedderLoadEvents to also fire the error event to the embedder element. r?nika

Revision D70585 was moved to bug 1627971. Setting attachment 9139899 [details] to obsolete.

Comment 6

[Phabricator Automation]

Comment on attachment 9139901 [details]
Bug 1629201 - Fire onerror event for CONTENT_BLOCKED in EndPageLoad. r?nika,ckerschb

Revision D70587 was moved to bug 1627971. Setting attachment 9139901 [details] to obsolete.

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to Tom Schuster [:evilpie] from comment #3)

Do note that it seems like Chrome for example often fires load events instead of error events, even when an error occurred: i.e. Bug 1601887, Bug 1418975, Bug 1552504.

Thanks!

(In reply to Olli Pettay [:smaug] from comment #4)

Yeah, need to be very careful when changing load and error event dispatching here.

Indeed! The intent of this bug is to try move the decision making to a central location so that it's possible to reason about the current state and so that we don't accidentally change behaviour with fission (which currently never fires error events for OOP iframes!).

Actually changing which cases result in load vs error is a whole different issue that we need to be super careful about.

Comment 8

[Chris Peterson [:cpeterson]]

Fission Future

Nice to have but doesn't block shipping Fission MVP.

Comment 9

[Neha Kochar [:neha]]

Should we do this in M8?

Comment 10

[Nika Layzell [:nika] (ni? for response)]

I think this is still a nice-to-have and shouldn't block shipping Fission unless we come across a specific issue caused by it.