New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 706260 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 695123
Owner:
Long OOO (go/where-is-mgiuca)
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

V8 IDL bindings generator: SecureContext + OriginTrialEnabled makes interface unconditional on origin trial status

Project Member Reported by mgiuca@chromium.org, Mar 29 2017

Issue description

I tried changing NavigatorInstalledApp.idl [1] "RuntimeEnabled=InstalledApp" to "OriginTrialEnabled=InstalledApp".

[
    OriginTrialEnabled=InstalledApp,
] partial interface Navigator {
    [SecureContext, CallWith=ScriptState, Measure]
    Promise<RelatedApplication> getInstalledRelatedApps();
};

This caused the method navigator.getInstalledRelatedApps to become unconditionally available (where previously it was gated on the runtime feature flag). It then crashes the renderer when called because Supplement<LocalFrame>::from() checks the origin trial flag and returns null.

V8NavigatorPartial is being generated incorrectly. Here is the current version [2]

  if (executionContext && (executionContext->isSecureContext())) {
    if (RuntimeEnabledFeatures::installedAppEnabled()) {
      // Install the method...
    }
  }

And after switching RuntimeEnabled to OriginTrialEnabled:

  if (executionContext && (executionContext->isSecureContext())) {
    // Install the method...
  }

The correct code to generate is actually NOTHING... according to the Origin Trial docs [3], you are currently supposed to manually write a condition to install the method.

Why does this affect me and not other origin trials (like WebShare [4])? It's SecureContext... for some reason, removing SecureContext makes this whole section (correctly) go away.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/installedapp/NavigatorInstalledApp.idl
[2] https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/V8NavigatorPartial.cpp?q=installedAppEnabled
[3] https://chromium.googlesource.com/chromium/src/+/master/docs/origin_trials_integration.md
[4] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webshare/NavigatorShare.idl
 

Comment 1 by mgiuca@chromium.org, Mar 29 2017

Mergedinto: 695123
Status: Duplicate (was: Started)

Sign in to add a comment