1377614 - System extensions fail to load in local builds

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, enhancement)

Description

[Haik Aftandilian [:haik]]

With the fix for 1334550, system WebExtensions (such as the Screenshot extension) fail to load their moz-extension resources. Verified on Mac and Linux, but Windows is TBD.

This is due to symlinks being used in the extension directory.

As an example, for the Screenshot extension, the extension directory is

  $REPO/obj-dbg.noindex/dist/NightlyDebug.app/Contents/Resources/browser/features/[e-mail]/webextension

which includes

  icons/icon-starred-32.svg and other icon files

which is a symlink to 

  $REPO/browser/extensions/screenshots/webextension/icons/icon-starred-32.svg

This is rejected by ExtensionProtocolHandler::NewStream() because it requires that the resource file be within the extension directory to avoid security issues caused by an unpacked extension moz-extension URI leading outside the extension dir.

It would be better if system extensions weren't setup using symlinks during the build process, but we can detect this case and workaround it in the browser using ContentChild.cpp:IsDevelopmentBuild() and MOZ_DEVELOPER_REPO_DIR.

Comment 1

[Haik Aftandilian [:haik]]

This doesn't affect Windows builds because they don't use symlinks and instead copy files from the repo to the extension directories.

For Mac and Linux, I looked into changing just the way extensions are setup to not use symlinks, but didn't see a simple method to do that that wouldn't also affect the full build omni jar. (We do it this way to speed up the build and make the dev/test cycle quicker, but fixing this for extensions shouldn't impose much overhead.) I'll file a follow-up to fix this in the build system.

This is similar to the problem we have with automated tests on Mac and Linux that use chrome:// URL's that resolve to files in the object dir that are symlinks to test files in the repo. We workaround this in the sandbox by allowing the content process to access files in the repo when run with certain environment variables/prefs that "mach" sets up.

For the fix, I'm going to do something similar for unpacked extension resources. i.e., on dev builds, the resource is allowed if it is in the repo dir. This doesn't weaken the sandbox because the content process is already given read access to the repo dir in dev builds.

Comment 2

[Haik Aftandilian [:haik]]

Attachment: Bug 1377614 - Part 1 - Move IsDevelopmentBuild() to common code.

Review commit: https://reviewboard.mozilla.org/r/154026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154026/

Comment 3

[Haik Aftandilian [:haik]]

Attachment: Bug 1377614 - Part 2 - System extensions fail to load on Mac and Linux local builds.

On Mac and Linux, allow unpacked extension resources to load from outside the extension directory if they're in the repo.

Review commit: https://reviewboard.mozilla.org/r/154028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154028/

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8883068 [details]
Bug 1377614 - Part 1 - Move IsDevelopmentBuild() to common code.

https://reviewboard.mozilla.org/r/154026/#review159164

Comment 6

[Haik Aftandilian [:haik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=310921294233baf86f329365923abca5ba812bc4
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48560b95a73f6536cd2bcfe2eff49a18d6f0dab6

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8883069 [details]
Bug 1377614 - Part 2 - System extensions fail to load on Mac and Linux local builds.

https://reviewboard.mozilla.org/r/154028/#review159674

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:508
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
> +#if !defined(XP_WIN)
> +// The |aRequestedFile| argument must already be Normalize()'d

I'd move this comment to the header comment, seems like something callers should know about.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:533
(Diff revision 1)
> +  }
> +
> +  if (mDevRepo) {
> +    // This is a development build
> +    bool isFromRepoDir = false;
> +    NS_TRY(mDevRepo->Contains(aRequestedFile, &isFromRepoDir));

I'd pass aResult to Contains. The compiler is going to optimize isFromRepoDir out anyway so not a big deal, but it'll make your code look cleaner.

Comment 8

[Haik Aftandilian [:haik]]

Comment on attachment 8883068 [details]
Bug 1377614 - Part 1 - Move IsDevelopmentBuild() to common code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154026/diff/1-2/

Comment 9

[Haik Aftandilian [:haik]]

Comment on attachment 8883069 [details]
Bug 1377614 - Part 2 - System extensions fail to load on Mac and Linux local builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154028/diff/1-2/

Comment 10

[Haik Aftandilian [:haik]]

Comment on attachment 8883069 [details]
Bug 1377614 - Part 2 - System extensions fail to load on Mac and Linux local builds.

https://reviewboard.mozilla.org/r/154028/#review159674

> I'd move this comment to the header comment, seems like something callers should know about.

I added an additional comment in the header file so this should be hard to miss.

> I'd pass aResult to Contains. The compiler is going to optimize isFromRepoDir out anyway so not a big deal, but it'll make your code look cleaner.

Done.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9137ed21a66f
Part 1 - Move IsDevelopmentBuild() to common code. r=Alex_Gaynor
https://hg.mozilla.org/integration/autoland/rev/0de6a6316120
Part 2 - System extensions fail to load on Mac and Linux local builds. r=jimm

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage: not finding mozilla/SandboxSettings.h at netwerk/protocol/res/ExtensionProtocolHandler.cpp:46:

https://hg.mozilla.org/integration/autoland/rev/2725a8a5b7c70960df6b90619772e4ad36be8bd3
https://hg.mozilla.org/integration/autoland/rev/d68b22db99cc5687aa604d4e5e32070c50f5cba7

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0de6a631612091b00fb3a319d0da73ec4cb39a58&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112062626&repo=autoland
>  /home/worker/workspace/build/src/netwerk/protocol/res/ExtensionProtocolHandler.cpp:46:10: fatal error: 'mozilla/SandboxSettings.h' file not found

Comment 13

[Haik Aftandilian [:haik]]

The Android and ASAN Linux build failures were hit because use of SandboxSettings.h should be behind an #ifdef MOZ_CONTENT_SANDBOX.

Comment 14

[Haik Aftandilian [:haik]]

Comment on attachment 8883068 [details]
Bug 1377614 - Part 1 - Move IsDevelopmentBuild() to common code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154026/diff/2-3/

Comment 15

[Haik Aftandilian [:haik]]

Comment on attachment 8883069 [details]
Bug 1377614 - Part 2 - System extensions fail to load on Mac and Linux local builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154028/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f718dc440c47
Part 1 - Move IsDevelopmentBuild() to common code. r=Alex_Gaynor
https://hg.mozilla.org/integration/autoland/rev/ed409a64e47b
Part 2 - System extensions fail to load on Mac and Linux local builds. r=jimm

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f718dc440c47
https://hg.mozilla.org/mozilla-central/rev/ed409a64e47b