Closed Bug 1417017 Opened 4 years ago Closed 4 years ago

no-cpows-in-tests misses content.* usage unless at global scope

Categories

(Firefox Build System :: Lint and Formatting, defect, P3)

defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file)

I believe the following `browser/` files are using actual cpows in tests:

https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_bug581253.js#27
  27:3  error  content.location is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_advanced_update.js#117
  117:23  error  content.gSubDialog._preloadDialog._overlay is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_cookies_dialog.js#43
  43:22  error  content.gSubDialog._dialogs[0]._box is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_password_management.js#69
  69:22  error  content.gSubDialog._dialogs[0]._box is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_siteData.js#150
  150:16  error  content.gSubDialog._topDialog is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_siteData3.js#140
  140:16  error  content.gSubDialog._topDialog is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_site_login_exceptions.js#74
  74:22  error  content.gSubDialog._dialogs[0]._box is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_subdialogs.js#182
  182:3  error  content.window.gSubDialog._topDialog.close is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/components/search/test/browser_abouthome_behavior.js#42
  42:17  error  gBrowser.contentDocumentAsCPOW is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/test/browser/browser_onboarding_skip_tour.js#39
  39:7  error  content.document.querySelector is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)
Comment on attachment 8928121 [details]
Bug 1417017 - Check for content. usage at global and within add_task for no-cpows-in-tests.

Does this approach make sense? And the errors from comment 0 are indeed errors that need to be fixed / eslint-disabled?
Attachment #8928121 - Flags: feedback?(standard8)
Priority: -- → P3
Mike, can you confirm that things like content.gSubDialog and content.location are CPOWs (or I guess, just content.*)?

I think that was one of the original intents of the rule, but it looks like I lost it somewhere.

On a side note, do we have a list of all the possible CPOW APIs?

Thanks
Flags: needinfo?(mconley)
gBrowser.contentDocumentAsCPOW and gBrowser.contentWindowAsCPOW are definitely CPOWs. Accessing properties on them should be banned.

Yeah, accessing `content` or any of its properties in a mochitest should be banned too - although we probably have a ton of violations that we need to whitelist.

(In reply to Mark Banner (:standard8) from comment #5)
> 
> On a side note, do we have a list of all the possible CPOW APIs?
> 

Thankfully, yes, kinda - accessing CPOWs like this has been banned in the front-end for over a year, but the add-on shims allow mochitest tests to use CPOWs in the bad way, and the shims themselves have a nice list of properties that they, er, shim.

toolkit/components/addoncompat/RemoteAddonsParent.jsm kind of encodes this logic. It's really complex and dense, but here are the things I'm seeing (this is an incomplete list - you might want to talk to gabor to see if you can get a full list):

(remote-)browser.docShell
(remote-)browser.sessionHistory
(remote-)browser.contentWindow
(remote-)browser.contentDocument
gBrowser.contentWindow
gBrowser.contentDocument
gBrowser.sessionHistory
gBrowser.docShell
window.content
browser.webNavigation.sessionHistory
Flags: needinfo?(mconley)
Thanks Mike, we've got most of those, though it looks like we should add the sessionHistory ones as well.
Comment on attachment 8928121 [details]
Bug 1417017 - Check for content. usage at global and within add_task for no-cpows-in-tests.

https://reviewboard.mozilla.org/r/199372/#review205932

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js:25
(Diff revision 3)
> -  /^gBrowser\.selectedBrowser\.contentWindow$/,
> -  /^browser\.contentDocument$/,
> -  /^window\.content$/
> +  /^gBrowser\.selectedBrowser\.contentWindow/,
> +  /^browser\.contentDocument/,
> +  /^window\.content/
>  ];
>  
> +var lastErrorStart;

Please add some documentation around this, what it is for and when it gets reset - it isn't immediately obvious when starting to look at the file.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js:127
(Diff revision 3)
>        });
> -      if (!someCpowFound && helpers.getIsGlobalScope(context.getAncestors())) {
> -        if (/^content\./.test(expression)) {
> -          showError(node, expression);
> +
> +      // Specially scope checks for "context." to avoid false positives.
> +      if (!someCpowFound && /^content\./.test(expression)) {
> +        // Don't error if we're multiple scopes deep. We only care about 2
> +        // scopes: global (length = 0) or immediately called by add_task.

I'm not sure I agree with this. Someone could do something like:

```
add_task(async () => {
  function foo() {
    Assert.ok(content.document);
  }
  foo();
});
```

or:

```
function foo() {
  Assert.ok(content.document);
}

add_task(async() => {
  foo();
});
```

I think both of those cases we'd want to catch. The only ones we wouldn't is if the function was used for a frame script, but that's harder to detect.
Attachment #8928121 - Flags: review?(standard8)
(In reply to Mark Banner (:standard8) from comment #8)
> > +        // Don't error if we're multiple scopes deep. We only care about 2
> > +        // scopes: global (length = 0) or immediately called by add_task.
> I'm not sure I agree with this. Someone could do something like:
Oh I'm well aware that it won't catch many possible cases. I was trying to reduce the false negatives. I was limiting it to directly inside add_task as that's similar to the global scope in that we know that can't be in the content scope. Things definitely get trickier when helper functions are defined in head.js or elsewhere:

function head_foo() {
  Assert.ok(content.document);
}

add_task(() => head_foo());

vs

add_task(() => ContentTask(…, head_foo));

Now the more exact solution would need to know that head_foo is a content-using function, so depending on how it's called, it could be an error, e.g., the appropriate argument position to ContentTask. But it gets even more complicated:

function head_bar() {
  ContentTask(…, head_foo);
}

add_task(() => head_bar());

vs

function head_bad() {
  head_foo();
}

add_task(() => head_bad());

Even focusing on the functions defined in a given file, we might need multiple passes (??) to know whether a function uses `content`, e.g., it's defined after use but hoisted, so that we can check if it's call / use would be allowed (i.e., not use CPOWs).

Standard8, did you have something in mind to address these types of potential false positives? I'm not familiar with eslint rule capabilities, but this seems to be getting into full compiler territory as opposed to just AST level. ?
Flags: needinfo?(standard8)
Er. The "more complicated" example should have been passing in `head_foo`, e.g.,

function head_maybe(foo, bar) {
  foo();
  ContentTask(…, bar)
}

add_task(() => head_maybe(ok, head_foo));

vs

add_task(() => head_maybe(head_foo, ok));
Hmm, thinking about it a bit more, lets go with the current setup - that should catch most items. Once we head more towards disabling cpows completely, we can think about more complicated cases.

Could you expand on the comment though please? Something like 'we only currently care about...' and a reference to why we're not catching the other items.
Flags: needinfo?(standard8)
Comment on attachment 8928121 [details]
Bug 1417017 - Check for content. usage at global and within add_task for no-cpows-in-tests.

https://reviewboard.mozilla.org/r/199372/#review205932

> Please add some documentation around this, what it is for and when it gets reset - it isn't immediately obvious when starting to look at the file.

Added comments here and for `isInContentTask` too

> I'm not sure I agree with this. Someone could do something like:
> 
> ```
> add_task(async () => {
>   function foo() {
>     Assert.ok(content.document);
>   }
>   foo();
> });
> ```
> 
> or:
> 
> ```
> function foo() {
>   Assert.ok(content.document);
> }
> 
> add_task(async() => {
>   foo();
> });
> ```
> 
> I think both of those cases we'd want to catch. The only ones we wouldn't is if the function was used for a frame script, but that's harder to detect.

Added comments explaining the "For now" behavior with ideal implementation.
Blocks: 1419623
Comment on attachment 8928121 [details]
Bug 1417017 - Check for content. usage at global and within add_task for no-cpows-in-tests.

https://reviewboard.mozilla.org/r/199372/#review207396

Great, thank you. You might just want to check for any new issues in the latest code before you push it though.
Attachment #8928121 - Flags: review?(standard8) → review+
(In reply to Mark Banner (:standard8) from comment #14)
> Great, thank you. You might just want to check for any new issues in the
> latest code before you push it though.
Nod. I did rebase this to latest master earlier, but I'll check against latest autoland too.
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/257de1de1304
Check for content. usage at global and within add_task for no-cpows-in-tests. r=standard8
https://hg.mozilla.org/mozilla-central/rev/257de1de1304
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.