Open Bug 1629201 Opened 2 years ago Updated 2 months ago

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

Categories

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

task

Tracking

()

Fission Milestone Future

People

(Reporter: mattwoodrow, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 obsolete files)

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.

See Also: → 1440212
See Also: → 1627971
Assignee: nobody → matt.woodrow
Status: NEW → ASSIGNED

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.

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

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.

Attachment #9139899 - Attachment is obsolete: true

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.

Attachment #9139901 - Attachment is obsolete: true

(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.

Assignee: matt.woodrow → nobody
Status: ASSIGNED → NEW

Fission Future

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

Severity: -- → normal
Type: defect → task
Fission Milestone: --- → Future
Priority: -- → P3

Should we do this in M8?

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.