Closed Bug 864845 Opened 11 years ago Closed 7 years ago

Make window.content chrome-only in nightly

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bholley, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files, 2 obsolete files)

part 1. Stop using 'content' in devtools tests
31.96 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
part 2. Make window.content chromeonly in nightly
4.91 KB, patch
nika
: review+
Details | Diff | Splinter Review 
This is the more proper fix for bug 857555, but we're going to start with the low-risk fix there.
Blocks: 863390
Note that I'm going to try to just remove window._content over in bug 946564.
Depends on: 946564
Bug 946564 made window._content chromeonly.
Summary: Make window.content and window._content chrome-only → Make window.content chrome-only
Blocks: stdglobal
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
No, not a duplicate.  Please actually read the summaries.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attached patch Make window.content chromeonly (obsolete) — — Splinter Review 

        Attachment #8905661 -
        Flags: review?(michael)

    
  
Assignee: nobody → bzbarsky
Status: REOPENED → ASSIGNED

        Attachment #8905661 -
        Flags: review?(michael) → review+

    
    

    
  
Hmm.  There's a ton of devtools tests that do stuff like:

      let textNodeName = yield testActor.eval(`
        content.document.querySelector("#badMarkup1").nextSibling.nodeName
      `);

or

  let expectedURL = yield testActor.eval(`
    content.document.querySelector(".canvas").toDataURL();`);

apparently in untrusted code.

What should those actually be doing?  Are they being evaluated against the relevant window anyway, such that they can drop the "content." part?
Flags: needinfo?(pbrosset)

    
    

    
  
I think those are incorrect, we should drop the `content.` part.
Forwarding the Alex in case I'm missing something.
Flags: needinfo?(pbrosset) → needinfo?(poirot.alex)

    
  

        Attachment #8905661 -
        Attachment is obsolete: true

    
  
Flags: needinfo?(poirot.alex)

    
    

    
  
Comment on attachment 8906208 [details] [diff] [review]
part 1.  Stop using 'content' in devtools tests

Review of attachment 8906208 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Yes, these testActor.eval end up being executed in a pure content sandbox:
  http://searchfox.org/mozilla-central/source/devtools/client/shared/test/test-actor.js#680-682

We have these "content." everywhere as these evaled scripts used to be frame scripts,
and we didn't throught about removing these using "content." while converting from frame script to testActor.eval pattern.

        Attachment #8906208 -
        Flags: review?(poirot.alex) → review+

    
    

    
  
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb8456c34de
part 1.  Stop using 'content' in devtools tests.  r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/27f3e967a340
part 2.  Make window.content chromeonly.  r=mystor

    
    

    
  
The returned green for the Talos test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e8ca7364e9c81b3190f19a3a44d2097b52b3474

Backed out for T-e10s(s) for tscrollx failure:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9d5cfcdb60333a098297d05b443df47a09c5b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f22bb948d17d638d22be937104b0246bf3ed586

First push which ran failing tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=296f26c8422126c7e80812173ebc38e548566234&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129776313&repo=mozilla-inbound

03:42:48     INFO -  TEST-INFO | started process 8302 (/builds/slave/test/build/application/firefox/firefox -profile /tmp/tmp0ryebz/profile -tp file:/builds/slave/test/build/tests/talos/talos/tests/scroll/scroll.manifest.develop -tpnoisy -tpcycles 1 -tppagecycles 25)
03:42:49     INFO -  PID 8302 |
03:42:49     INFO -  PID 8302 | (/builds/slave/test/build/application/firefox/firefox:8352): Pango-WARNING **: error opening config file '/home/cltbld/.pangorc': Permission denied
03:42:49     INFO -  PID 8302 |
03:42:59     INFO -  PID 8302 | RSS: Main: 173174784
03:42:59     INFO -  PID 8302 |
04:42:48     INFO -  Timeout waiting for test completion; killing browser...
04:42:48     INFO -  Terminating psutil.Process(pid=8302, name='firefox')
04:42:48     INFO -  PID 8302 | ExceptionHandler::GenerateDump cloned child 8488
04:42:48     INFO -  PID 8302 | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
04:42:48     INFO -  PID 8302 | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
04:42:48     INFO -  TEST-UNEXPECTED-ERROR | tscrollx | timeout
04:42:48    ERROR -  Traceback (most recent call last):
04:42:48     INFO -    File "/builds/slave/test/build/tests/talos/talos/run_tests.py", line 281, in run_tests
04:42:48     INFO -      talos_results.add(mytest.runTest(browser_config, test))
04:42:48     INFO -    File "/builds/slave/test/build/tests/talos/talos/ttest.py", line 61, in runTest
04:42:48     INFO -      return self._runTest(browser_config, test_config, setup)
04:42:48     INFO -    File "/builds/slave/test/build/tests/talos/talos/ttest.py", line 209, in _runTest
04:42:48     INFO -      if counter_management else None),
04:42:48     INFO -    File "/builds/slave/test/build/tests/talos/talos/talos_process.py", line 130, in run_browser
04:42:48     INFO -      raise TalosError("timeout")
04:42:48     INFO -  TalosError: timeout

    
    

    
  
Ah, the perils of only running out correctness tests...

The harness for T-e10s(s), in testing/talos/talos/pageloader/chrome/tscroll.js, has this bit:

  function rAF(fn) {
    return content.requestAnimationFrame(fn);
  }

which failed with these patches, because it runs untrusted.  Just dropping the "content." makes it work fine.
Flags: needinfo?(bzbarsky)

    
    

    
  
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e6a4d96cf76
part 1.  Stop using 'content' in devtools tests.  r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/c991b71d3642
part 2.  Make window.content chromeonly.  r=mystor

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9e6a4d96cf76
https://hg.mozilla.org/mozilla-central/rev/c991b71d3642
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

    
    

    
  
Backed out for causing bug 1399649. Guess the tscroll issues weren't entirely solved yet :(

https://hg.mozilla.org/mozilla-central/rev/c15e2f280729b6503f9455cd4448ab2852eb5806
Status: RESOLVED → REOPENED

          status-firefox57:
          fixed → ---
Flags: needinfo?(bzbarsky)
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---

    
    

    
  
I believe Talos only runs from source on try...  For other branches, you need to also sign and update XPI for the test.  :jmaher should know for sure.
Flags: needinfo?(jmaher)

    
    

    
  
on try server and locally we do not run the .xpi, but the source- which means signing and packaging are not needed until ready.

to sign extensions:
https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions#Signing_an_Addon

to force Talos to use the .xpi, remove this condition:
http://searchfox.org/mozilla-central/source/testing/talos/talos/ffsetup.py#95

likewise to force Talos to use a signed extension remove this condition:
http://searchfox.org/mozilla-central/source/testing/talos/talos/run_tests.py#121
Flags: needinfo?(jmaher)

    
    

    
  
So wait, I'm confused.  For the Ts test tryserver is basically a lie?  And so is inbound?  Or is there something else going on?  This was green on try and on inbound....

What are the actual steps for updating whatever other thing all the other branches use?  :(
Flags: needinfo?(jmaher)

    
    

    
  
I am not 100% sure why this broke on the uplift simulation to mozilla-beta.  I believe we have different configurations in the browser and maybe prefs that are different and more restrictive.

Try server is good for testing results and with the exception of a fileIO measurement (because try uses by default the raw files for the .xpi files), try results can be compared against mozilla-central or mozilla-inbound/autoland.

:ryanvm, any tips on determining what is different on mozilla-beta?
Flags: needinfo?(jmaher) → needinfo?(ryanvm)

    
    

    
  
No, I'm not aware of how the signing requirements vary. I posted links to the two patches used to reproduce the failures over in bug 1399649, though. Feel free to look them over.
Flags: needinfo?(ryanvm)

    
  
Flags: needinfo?(jmaher)

    
    

    
  
So wait, just so we're clear...  T-e10s(s), which is the thing I made sure to test, passes on that "trunk as late beta" push.  What fails is T-e10s(g1), in the tp5o_scroll test.

The tscroll.js file I modified says:

  // Note: This file is used at both tscrollx and tp5o_scroll. With the former as
  //       unprivileged code.

but it's possible that this comment is a lie and _both_ are unprivileged ... except on late beta.  Or something.  It's hard to say, because both tp5o_scroll and tscrollx do a great job of hiding any error output, so figuring out why they fail is exciting.

    
    

    
  
that comment was written well over a year ago, we have changed so much in Firefox since then- and while I make sure Talos runs and reports useful numbers, I don't know much about the tests themselves- they are developer written and maintained.

I also don't understand the problem here- I see that tp5o_scroll failed when run on mozilla-beta.  Do we know this is related to signing?  Could it be another configuration change?

I suspect the tp5o_scroll needs to sign pageloader addon since that has tscroll.js code in it.  If we landed code to change tscroll.js on inbound and didn't rebuild/sign pageloader.xpi then we have been testing without the tscroll.js changes on mozilla-inbound for the 22 hours the patch existed.  Possibly a config setting in mozilla-beta enforced different restrictions.

:ryanvm- I wanted to see a list of what is different between mozilla-beta and mozilla-inbound, not just signing.
Flags: needinfo?(jmaher)

    
    

    
  
How the pageloader addon can run on beta where non-WE addons are forbidden?

    
    

    
  
we support them with certain preferences such as |extensions.legacy.enabled=true|:
http://searchfox.org/mozilla-central/source/testing/talos/talos/config.py#197

there is more data here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1361002#c10

    
    

    
  
> they are developer written and maintained.

Do you know who the developers maintaining tp5o_scroll are?

> Do we know this is related to signing?

No.

> Could it be another configuration change?

Yes.  In fact, I'm doing some try experiments right now to test some other hypotheses.  It's hard to tell what to try because of the lack of useful error reporting.  :(

> If we landed code to change tscroll.js on inbound and didn't rebuild/sign pageloader.xpi 

Where does pageloader.xpi live?  I don't see it in-tree...

> then we have been testing without the tscroll.js changes on mozilla-inbound for the 22 hours the patch existed.

To be clear, tscrollx fails on inbound without that change.  So at least for purposes of tscrollx the change is being picked up.

I don't know about tp5o_scroll on inbound and whether it's been picking up the change...
Flags: needinfo?(jmaher)

    
    

    
  
well the developers who worked on it in the past have since left Mozilla- we have no owner for this test.

pageloader is testing/talos/talos/pageloader/pageloader-signed.xpi.

talos should report all stderr/stdout in the log file or on a local run.

I will be traveling today, so my responses will be limited for the next day- I added :rwood here who could answer some technical details about talos as needed.
Flags: needinfo?(jmaher)

    
    

    
  
> we have no owner for this test.

I see.  Do we have a process for finding owners for tests that become unowned?  (If not, I can push for such a process to exist; owners should be teams, not individuals, like any other code.)

> pageloader is testing/talos/talos/pageloader/pageloader-signed.xpi.

Thank you.  I'll look into this bit.

> talos should report all stderr/stdout in the log file or on a local run.

JS exceptions are not logged to stderr/stdout except in debug builds, and we apparently don't run talos in debug builds.  Which makes diagnosing talos failures painful, of course.  :(

    
    

    
  
OK.  So I dunno that pageloader-signed.xpi is really relevant at all.

As far as I can tell, here's what's going on:

1) tscroll.js is used in both the tscrollx test and the tp5o_scroll test.
2) In tscrollx it's used in a "non-chrome window" environment, so "content.requestAnimationFrame"
   throws, but bareword "requestAnimationFrame" is ok.
3) In tp5o_scroll is used as a frame script, via the messageManager.loadFrameScript call in
   testing/talos/talos/pageloader/chrome/pageloader.js.

In all cases, the version I'm actually modifying seems to be being used, not the one in pageloader-signed.xpi, afaict.

On inbound/m-c/try the global the frame script is loaded into seems to have a requestAnimationFrame.  In the beta try pushes it does not.

I'm still digging to see why that difference, but in the meantime simply using win.requestAnimationFrame (using whatever window it is we're supposed to be scrolling) makes both tscrollx and tp5o_scroll happy.  See https://treeherder.mozilla.org/#/jobs?repo=try&revision=d85f1edaaf13c5180515983f464301a7f976dab5

    
    

    
  
Oh, and the point is that frame script globals are nsIContentFrameMessageManager instances and hence have a "content".

    
    

    
  
And just in case it isn't clear, this totally reproduces locally (I thought it was my fault on a recent try push until I found this bug). ./mach talos-test --suite g1-e10s

    
    

    
  
I was going to be really happy and try actually figuring out where the extra requestAnimationFrame comes from, but when I run that command it dies:

22:40:23    ERROR - Exception during post-action for create-virtualenv: Traceback (most recent call last):
...
22:40:23    ERROR - IOError: [Errno 2] No such file or directory: '/home/bzbarsky/mozilla/vanilla/obj-firefox/testing/talos-venv/bin/activate_this.py'
Flags: needinfo?(rwood)

    
    

    
  


  
OK, let's just do this nightly-only for now.  I'll file a followup for doing it on all channels.  This should be green no matter what, though.

        Attachment #8908466 -
        Flags: review?(michael)

    
  
Flags: needinfo?(bzbarsky)

    
  

        Attachment #8906209 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8908466 [details] [diff] [review]
part 2.  Make window.content chromeonly in nightly

Review of attachment 8908466 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/bugs/test_bug440572.html
@@ +28,5 @@
>  function runtests()
>  {
>    is(messages.size, 3, "received the right number of messages.");
>    is(messages.get("test"), "success", "test in frame failed.");
> +  is(messages.get("content"), "success", "parent[\"content\"] should be the subframe named 'content'");

Won't this test start failing when we merge into beta?

::: dom/webidl/Window.webidl
@@ +335,5 @@
>                                                                     optional DOMString name = "",
>                                                                     optional DOMString options = "",
>                                                                     any... extraArguments);
>  
> +  [Func="nsGlobalWindow::IsContentPropertyEnabled", Replaceable, Throws, NeedsCallerType] readonly attribute object? content;

I think this line is long enough now we should probably wrap after the ]

I also think that Window.webidl is already a preprocessed webidl file, so we could even just ifdef a `[ChromeOnly]` attribute in on NIGHTLY_BUILD directly in this file and skip the function. Kinda gross but *shrug*.

        Attachment #8908466 -
        Flags: review?(michael) → review+

    
    

    
  
> Won't this test start failing when we merge into beta?

Argh.  Yes, it will.  I don't have any clever suggestions here apart from removing that line altogether; do you?

> I also think that Window.webidl is already a preprocessed webidl file, so we could even just ifdef a
> `[ChromeOnly]` attribute in on NIGHTLY_BUILD directly in this file and skip the function.

Hmm.  Yeah, that would work.  I'll do that.
Flags: needinfo?(michael)

    
  
Blocks: 1400140

    
  
Summary: Make window.content chrome-only → Make window.content chrome-only in nightly

    
    

    
  
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #35)
> I was going to be really happy and try actually figuring out where the extra
> requestAnimationFrame comes from, but when I run that command it dies:
> 
> 22:40:23    ERROR - Exception during post-action for create-virtualenv:
> Traceback (most recent call last):
> ...
> 22:40:23    ERROR - IOError: [Errno 2] No such file or directory:
> '/home/bzbarsky/mozilla/vanilla/obj-firefox/testing/talos-venv/bin/
> activate_this.py'

Hmmm, haven't seen this before. Maybe try removing the talos-venv folder and try again? And I'm assuming './mach run' works correct (i.e. the build starts fine?)
Flags: needinfo?(rwood)

    
    

    
  
> Maybe try removing the talos-venv folder and try again?

Then it fails with:

14:59:18     INFO -  Installing setuptools, pip, wheel...
14:59:18     INFO -    Complete output from command /home/bzbarsky/mozil...s-venv/bin/python2.7 - setuptools pip wheel:
14:59:18    ERROR -    Traceback (most recent call last):
14:59:18     INFO -    File "<stdin>", line 4, in <module>
14:59:18     INFO -    File "/usr/lib64/python2.7/tempfile.py", line 32, in <module>
14:59:18     INFO -      import io as _io
14:59:18     INFO -    File "/usr/lib64/python2.7/io.py", line 51, in <module>
14:59:18     INFO -      import _io
14:59:18     INFO -  ImportError: No module named _io
14:59:18    ERROR -  Traceback (most recent call last):
14:59:18     INFO -    File "/home/bzbarsky/mozilla/vanilla/mozilla/third_party/python/virtualenv/virtualenv.py", line 2325, in <module>
14:59:18     INFO -      main()
14:59:18     INFO -    File "/home/bzbarsky/mozilla/vanilla/mozilla/third_party/python/virtualenv/virtualenv.py", line 711, in main
14:59:18     INFO -      symlink=options.symlink and hasattr(os, 'symlink')) # MOZ: Make sure we don't use symlink when we don't have it
14:59:18     INFO -    File "/home/bzbarsky/mozilla/vanilla/mozilla/third_party/python/virtualenv/virtualenv.py", line 944, in create_environment
14:59:18     INFO -      download=download,
14:59:18     INFO -    File "/home/bzbarsky/mozilla/vanilla/mozilla/third_party/python/virtualenv/virtualenv.py", line 900, in install_wheel
14:59:18     INFO -      call_subprocess(cmd, show_stdout=False, extra_env=env, stdin=SCRIPT)
14:59:18     INFO -    File "/home/bzbarsky/mozilla/vanilla/mozilla/third_party/python/virtualenv/virtualenv.py", line 795, in call_subprocess
14:59:18     INFO -      % (cmd_desc, proc.returncode))
14:59:18     INFO -  OSError: Command /home/bzbarsky/mozil...s-venv/bin/python2.7 - setuptools pip wheel failed with error code 1

> And I'm assuming './mach run' works correct

Yes.
Flags: needinfo?(rwood)

    
    

    
  
I really don't know, sorry :( only thing I can suggest is try re-installing python setup-tools (?) looks like that is failing for some reason.
Flags: needinfo?(rwood)

    
    

    
  
Right, but what reason?

Fwiw, if I just "rm -rf /home/bzbarsky/mozilla/vanilla/obj-firefox/testing/talos-venv && virtualenv /home/bzbarsky/mozilla/vanilla/obj-firefox/testing/talos-venv" then things work fine; I can ./mach talos-test.  So it's not a problem with my virtualenv in general; there's just some problem with how we're invoking it from talos-test.
Flags: needinfo?(rwood)

    
    

    
  
OK.  By manually running virtualenv, I managed to run tp5o_scroll locally, on tip.  When I do that, the bareword "requestAnimationFrame" does NOT work, which matches expectations.  I have no idea how it managed to be green on try/inbound/m-c, nor how what those do differs from "mach talos-test -a tp5o_scroll".  :(

I'm going to modify the test to accept both results, I guess, do the ifdef thing in Window.webidl, and check this in.
Flags: needinfo?(michael)

    
    

    
  
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd97f24d9ae7
part 1.  Stop using 'content' in devtools tests.  r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f52a45c01dc
part 2.  Make window.content chromeonly in nightly.  r=mystor

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/cd97f24d9ae7
https://hg.mozilla.org/mozilla-central/rev/0f52a45c01dc
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Flags: needinfo?(rwood)

    
    

    
  
Robert, Joel,

I filed bug 1401190 on the issues running talos locally.

    
    

    
  
Chris, this change is nightly-only for now; release is unaffected.  So I'm not sure we should be relnoting it for 57.  Bug 1400140 tracks removing window.content in release.

    
    

    
  
Chris, see comment 49.
Flags: needinfo?(cmills)

    
    

    
  
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #50)
> Chris, see comment 49.

D'oh, sorry about that. I've removed it, updated the notes, and added a note to the experimental features page.
Flags: needinfo?(cmills)

    
    

    
  
No need to be sorry.  It was pretty confusing, what happened in this bug.  ;)
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: