1637493 - Audit use of customUserAgent in content/Emulation.jsm and ensure only occurs from Parent process.

Status: RESOLVED FIXED

(Remote Protocol :: CDP, task, P3)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dan, what would have to be done to get this set from the parent process? Some advice would be appreciated. Right now we simply forward it to the content because we haven't found a way in doing it from the parent:

https://searchfox.org/mozilla-central/rev/0688ffdef223dac527c2fcdb25560118c4e4df51/remote/domains/parent/Emulation.jsm#107-123

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The needinfo still stands. I assume it was removed when you updated the summary.

Comment 3

[u480271]

Hi Henrik,

You can set the customUserAgent on the browsing context object instead of the docshell. I'm not very fluent in JS, but I think something like this should suffice:

async setUserAgentOverride(options = {}) {
    const { userAgent } = options;

    if (typeof userAgent != "string") {
      throw new TypeError(
        "Invalid parameters (userAgent: string value expected)"
      );
    }

    const { browsingContext } = this.session.target;

    if (userAgent.length == 0) {
      browsingContext.customUserAgent = null;
    } else if (this._isValidHTTPRequestHeaderValue(userAgent)) {
      browsingContext.customUserAgent = userAgent;
    } else {
      throw new TypeError("Invalid characters found in userAgent");
    }
  }

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so maybe you can explain the changes of your patch on bug 1633459, which reverts it from using the browsing context to docshell? Is it just because the setting happens in content instead of the parent process and which causes crashes?

Comment 5

[u480271]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #4)

Ok, so maybe you can explain the changes of your patch on bug 1633459, which reverts it from using the browsing context to docshell? Is it just because the setting happens in content instead of the parent process and which causes crashes?

The issue is that the docShell customUserAgent changes are kinda midway through refactoring. The property was moved from nsDocShell to BrowsingContext and then the callers to docShell.customUserAgent = were updated to docShell.browsingContext.customUserAgent =. This means that the setting still happens from the Content process. BrowsingContext is split across Content/Parent; the setting from Content is forwarded to the Parent. There's a test on the Content side to determine if it's OK to send and a check in the Parent side also. In Bug 1633459, the assert in firing in the Parent because a change is being made after the BrowsingContext has changed process and the sender is no longer allowed to set the override. (The Content side thought it was OK to send).

I put the IDL attribute back into nsIDocShell so I can reject setting the override if the doc shell knows it's losing rights to change it's BrowsingContext (The check to mWillProcessChange). This is just an interim fix to stop the assert until we can update all the uses of .customUserAgent = ... to originate from the Parent process, which always has rights to set the property on a BrowsingContext.

Comment 6

[u480271]

I made the changes locally and ran browser_setUserAgentOverride.js test and it appears to pass. Do you want me to upload my changes here?

Comment 7

[u480271]

Attachment: Bug 1637493 - [remote] Set customUserAgent from Parent process.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you for the detailed explanation and the already uploaded patch. Lets continue over there.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8a0b94a5165
[remote] Set customUserAgent from Parent process. r=remote-protocol-reviewers,whimboo

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c8a0b94a5165