1843846 - Background context tracking should not be confused by iframes

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Rob Wu [:robwu]]

The presence of iframes in background contexts can currently confuse the background context tracking logic as follows:

• expectViewLoad is currently called whenever DOMContentLoaded is dispatched. But because this event bubbles up from child frames to the frame message manager, it is possible for a completed child frame load to prematurely signal completion of the top-level load. Consequently, ext-backgroundPage.js may incorrectly believe the background context to have finished loading, and potentially miss registration of persistent listeners.

• ProxyContextParent is currently initialized on the first extension API call, or the DOMContentLoaded notification, whichever is first. If the child frame has an extension API call before the top-level background context, then the ProxyContextParent of the frame will be the first one with viewType = "background". That causes extension.backgroundContext to be incorrectly set to that child's context instead of the top-level context.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1843846 - Dispatch ExtensionViewLoaded only for top-level context

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1843846 - Ignore child frames in backgroundContext

context.isBackgroundContext is used to determine whether the
background context participates in the event page suspension mechanism.
This should exclude child frames, despite them being tagged as
background contexts as well.

To determine whether a context is the top context, the browsingContext
is forwarded from recvCreateProxyContext to the
ExtensionPageContextParent constructor. To minimize the extent of
changes, browsingContext is not stored in the class instance.
Only the following are stored, derived from browsingContext:

• xulBrowser (pre-existing)
• isTopContext

This patch also clarifies the expected types that are involved with
comments and assertions: The availability of browsingContext was
previously not obvious in the code due to the presence of ? in
actor.browsingContext?., introduced in part 1.6 of bug 1688040.

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1843846 - Add extra test coverage for getBackgroundPage()

The parent patches and bug are about issues with tracing the correct
context in the parent process. The current implementation in the child
already works correctly (even before the parent patches). To make sure
that this continues to work correctly in the future, add test coverage
for extension.getBackgroundPage().

Comment 4

[Rob Wu [:robwu]]

Linking more bugs for context:

• bug 1688040 introduced weakened conditions that the patch in comment 2 rectifies.
• Without the fix in this bug, event page timeout handling for filterResponseData (added in bug 1759300) does not work correctly under the circumstances sketched in the initial report.
• bug 1762225 relies on a correct ExtensionViewLoaded message for background context tracing. An incorrectly timed message can result in missing persistent listeners.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25556601cd14
Dispatch ExtensionViewLoaded only for top-level context r=rpl
https://hg.mozilla.org/integration/autoland/rev/271e26c0b265
Ignore child frames in backgroundContext r=rpl
https://hg.mozilla.org/integration/autoland/rev/aa0faf5572aa
Add extra test coverage for getBackgroundPage() r=rpl

Comment 6

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/25556601cd14
https://hg.mozilla.org/mozilla-central/rev/271e26c0b265
https://hg.mozilla.org/mozilla-central/rev/aa0faf5572aa