Bugzilla

Updated

•

7 years ago

Flags: needinfo?(amarchesini)

Bill McCloskey [inactive unless it's an emergency] (:billm)

Assignee

Comment 8

•

7 years ago

I'd like to try to revive the paint during GC project.  :-)  Taking this one.

Assignee: nobody → ehsan

Status: RESOLVED → REOPENED

Resolution: WONTFIX → ---

Comment 9

•

7 years ago

(In reply to :Ehsan Akhgari from comment #8)
> I'd like to try to revive the paint during GC project.  :-)  Taking this one.

Why?

Assignee

Comment 10

•

7 years ago

(In reply to Bill McCloskey (:billm) from comment #9)
> (In reply to :Ehsan Akhgari from comment #8)
> > I'd like to try to revive the paint during GC project.  :-)  Taking this one.
> 
> Why?

I'd like to try to extend the coverage of APZ, and one of the aspects that I'm looking at is reducing checkerboarding.

Our GC times are quite terrible, and while I'm hoping that we can work on improving both our GC times, and their scheduling, I'd like to have this as a band-aid of sorts for scrolling.  I've looked at the bugs that were left the last time we decided to back this out and it seems that with some work we should be able to fix them, so I'd like to try it out.

(Also, this bug on its own is valuable, see bug 1348460.)

Assignee

Comment 11

•

7 years ago

Attached patch Avoid going into SpiderMonkey for retrieving origin attributes (obsolete) — Details — Splinter Review

Our caller is C++ code, and the implementations are all also written in C++,
so there is no reason to go through SpiderMonkey here.  This patch also makes
nsILoadContext builtinclass to ensure that the implementation is always native.

Attachment #8848792 - Flags: review?(amarchesini)

Bill McCloskey [inactive unless it's an emergency] (:billm)

Assignee

Updated

•

7 years ago

Blocks: 1348460

Comment 13

•

7 years ago

(In reply to :Ehsan Akhgari from comment #10)
> (In reply to Bill McCloskey (:billm) from comment #9)
> > (In reply to :Ehsan Akhgari from comment #8)
> > > I'd like to try to revive the paint during GC project.  :-)  Taking this one.
> > 
> > Why?
> 
> I'd like to try to extend the coverage of APZ, and one of the aspects that
> I'm looking at is reducing checkerboarding.

Extending the painting stuff to cover APZ seems like a good idea to me. There's some good information in bug 1305130 if you haven't seen it already.

> Our GC times are quite terrible, and while I'm hoping that we can work on
> improving both our GC times, and their scheduling, I'd like to have this as
> a band-aid of sorts for scrolling.  I've looked at the bugs that were left
> the last time we decided to back this out and it seems that with some work
> we should be able to fix them, so I'd like to try it out.

We can do the APZ thing without fixing this GC stuff. I think doing APZ by itself would be a lot easier.

And despite our not great GC times, there isn't a lot of evidence that GC contributes much to tab switching problems. Turning off the GC stuff didn't help any of the telemetry measurements that we have for tab switch times or spinner duration. And the stacks that Mike has for tab switching jank don't show GC to be a major factor (please ask him for the bug# that has the latest stacks, I can't find it).

Given all the bugs with the GC stuff, this is not low hanging fruit. And it probably won't help much even if we fix it. So this just seems like a bad thing to work on to me.

> (Also, this bug on its own is valuable, see bug 1348460.)

Well, okay, that's fine.

Comment 14

•

7 years ago

Comment on attachment 8848792 [details] [diff] [review]
Avoid going into SpiderMonkey for retrieving origin attributes

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

::: docshell/base/nsILoadContext.idl
@@ +142,2 @@
>     */
> +  NS_IMETHOD_(bool) GetOriginAttributes(mozilla::OriginAttributes& aAttrs) = 0;

No reasons to return bool. void is enough if you change TabParent.

::: dom/ipc/TabParent.cpp
@@ +3012,5 @@
>    NS_IMETHOD GetIsInIsolatedMozBrowserElement(bool*) NO_IMPL
>    NS_IMETHOD GetOriginAttributes(JS::MutableHandleValue) NO_IMPL
> +  NS_IMETHOD_(bool) GetOriginAttributes(mozilla::OriginAttributes&) override
> +  {
> +    return false;

Also TabParent has an originAttributes. Just do:

aAttrs = OriginAttributesRef();
return true;

Attachment #8848792 - Flags: review?(amarchesini) → review+

Assignee

Comment 15

•

7 years ago

(In reply to Andrea Marchesini [:baku] from comment #14)
> Comment on attachment 8848792 [details] [diff] [review]
> Avoid going into SpiderMonkey for retrieving origin attributes
> 
> Review of attachment 8848792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/base/nsILoadContext.idl
> @@ +142,2 @@
> >     */
> > +  NS_IMETHOD_(bool) GetOriginAttributes(mozilla::OriginAttributes& aAttrs) = 0;
> 
> No reasons to return bool. void is enough if you change TabParent.

OK.

> ::: dom/ipc/TabParent.cpp
> @@ +3012,5 @@
> >    NS_IMETHOD GetIsInIsolatedMozBrowserElement(bool*) NO_IMPL
> >    NS_IMETHOD GetOriginAttributes(JS::MutableHandleValue) NO_IMPL
> > +  NS_IMETHOD_(bool) GetOriginAttributes(mozilla::OriginAttributes&) override
> > +  {
> > +    return false;
> 
> Also TabParent has an originAttributes. Just do:
> 
> aAttrs = OriginAttributesRef();
> return true;

This is FakeChannel, not TabParent. :-)

Assignee

Comment 16

•

7 years ago

(In reply to Bill McCloskey (:billm) from comment #13)
> We can do the APZ thing without fixing this GC stuff. I think doing APZ by
> itself would be a lot easier.

That's good to know, but I'd like to understand why better.  I'll try to ping you on IRC today to understand why.

> And despite our not great GC times, there isn't a lot of evidence that GC
> contributes much to tab switching problems. Turning off the GC stuff didn't
> help any of the telemetry measurements that we have for tab switch times or
> spinner duration. And the stacks that Mike has for tab switching jank don't
> show GC to be a major factor (please ask him for the bug# that has the
> latest stacks, I can't find it).

Sorry for the confusion, I wasn't really focusing on tab switches, I was (perhaps naively) assuming some of the bugs we hit with painting during GC when tab switching will happen when painting while GCing for APZ (which is of course the next step after we do bug 1305130.)  Perhaps my assumption there was incorrect?  I'll sync up with you on that.

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

Comment 17

•

7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fecc6abcf708
Avoid going into SpiderMonkey for retrieving origin attributes; r=baku

Comment 18

•

7 years ago

Backed out for bustage at docshell/base/SerializedLoadContext.cpp:65:61:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2857884dea0e7c17972fdda9eca35928722ffd08

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fecc6abcf708eb09f5af40f50ad0a825842e43a1&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=85341354&repo=mozilla-inbound

/home/worker/workspace/build/src/docshell/base/SerializedLoadContext.cpp:65:61: error: could not convert 'aLoadContext->nsILoadContext::GetOriginAttributes((* &((IPC::SerializedLoadContext*)this)->IPC::SerializedLoadContext::mOriginAttributes))' from 'void' to 'bool'
/home/worker/workspace/build/src/docshell/base/SerializedLoadContext.cpp:65:61: error: in argument to unary !

Flags: needinfo?(ehsan)

Assignee

Comment 19

•

7 years ago

Sorry, my local build was too slow, and I was too impatient.  :(

Flags: needinfo?(ehsan)

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

Comment 20

•

7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e47807067a6
Avoid going into SpiderMonkey for retrieving origin attributes; r=baku

Comment 21

•

7 years ago

Backed out for Windows bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f8950030a5b600e29df14da687ef35b70a5ce5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7e47807067a60a61a8cb205e240dbc679163b55b&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=85633193&repo=mozilla-inbound

14:12:46     INFO -  z:\build\build\src\uriloader\prefetch\OfflineCacheUpdateParent.h(57): error C2695: 'mozilla::docshell::OfflineCacheUpdateParent::GetOriginAttributes': overriding virtual function differs from 'nsILoadContext::GetOriginAttributes' only by calling convention
14:12:46     INFO -  z:\build\build\src\obj-firefox\dist\include\nsILoadContext.h(102): note: see declaration of 'nsILoadContext::GetOriginAttributes'
14:12:46     INFO -  z:/build/build/src/uriloader/prefetch/OfflineCacheUpdateParent.cpp(280): error C2373: 'mozilla::docshell::OfflineCacheUpdateParent::GetOriginAttributes': redefinition; different type modifiers
14:12:46     INFO -  z:\build\build\src\uriloader\prefetch\OfflineCacheUpdateParent.h(57): note: see declaration of 'mozilla::docshell::OfflineCacheUpdateParent::GetOriginAttributes'
14:12:46     INFO -  z:/build/build/src/config/rules.mk:1011: recipe for target 'Unified_cpp_uriloader_prefetch0.obj' failed
14:12:46     INFO -  mozmake.EXE[5]: *** [Unified_cpp_uriloader_prefetch0.obj] Error 2
14:12:46     INFO -  mozmake.EXE[5]: Leaving directory 'z:/build/build/src/obj-firefox/uriloader/prefetch'
14:12:46     INFO -  z:/build/build/src/config/recurse.mk:71: recipe for target 'uriloader/prefetch/target' failed
14:12:46     INFO -  mozmake.EXE[4]: *** [uriloader/prefetch/target] Error 2

Flags: needinfo?(ehsan)

Assignee

Comment 22

•

7 years ago

So sorry.  :(

Flags: needinfo?(ehsan)

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

Comment 23

•

7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef22dba0dac5
Avoid going into SpiderMonkey for retrieving origin attributes; r=baku

Comment 24

•

7 years ago

Backed out for various test failures, e.g. xpcshell netwerk/test/unit/test_bug826063.js and browser-chrome browser/components/downloads/test/browser/browser_iframe_gone_mid_download.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b30d5f5b423e6047eff9095c6f7181c331c8972

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ef22dba0dac560980462f670db732470d5e364c1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=85705269&repo=mozilla-inbound
[task 2017-03-22T19:08:39.724603Z] 19:08:39     INFO -  TEST-PASS | netwerk/test/unit/test_bug826063.js | test_setPrivate_regular - [test_setPrivate_regular : 38] false == false
[task 2017-03-22T19:08:39.726328Z] 19:08:39     INFO -  (xpcshell/head.js) | test run_next_test 3 pending (3)
[task 2017-03-22T19:08:39.728018Z] 19:08:39     INFO -  (xpcshell/head.js) | test test_setPrivate_regular finished (3)
[task 2017-03-22T19:08:39.729713Z] 19:08:39     INFO -  (xpcshell/head.js) | test run_next_test 2 finished (2)
[task 2017-03-22T19:08:39.731474Z] 19:08:39     INFO -  netwerk/test/unit/test_bug826063.js | Starting test_LoadContextPrivate
[task 2017-03-22T19:08:39.733200Z] 19:08:39     INFO -  (xpcshell/head.js) | test test_LoadContextPrivate pending (2)
[task 2017-03-22T19:08:39.740884Z] 19:08:39  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_bug826063.js | test_LoadContextPrivate - [test_LoadContextPrivate : 38] false == true
[task 2017-03-22T19:08:39.742842Z] 19:08:39     INFO -  /home/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_bug826063.js:checkPrivate:38
[task 2017-03-22T19:08:39.744677Z] 19:08:39     INFO -  /home/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_bug826063.js:test_LoadContextPrivate:82
[task 2017-03-22T19:08:39.746428Z] 19:08:39     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_run_next_test:1569
[task 2017-03-22T19:08:39.748141Z] 19:08:39     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:run:703
[task 2017-03-22T19:08:39.749851Z] 19:08:39     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_do_main:211
[task 2017-03-22T19:08:39.751592Z] 19:08:39     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:546
[task 2017-03-22T19:08:39.753240Z] 19:08:39     INFO -  -e:null:1
[task 2017-03-22T19:08:39.754906Z] 19:08:39     INFO -  exiting test

Failure log browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=85705286&repo=mozilla-inbound
[task 2017-03-22T19:03:32.135540Z] 19:03:32     INFO - TEST-START | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js
[task 2017-03-22T19:03:34.260942Z] 19:03:34     INFO - TEST-INFO | started process screentopng
[task 2017-03-22T19:03:35.478734Z] 19:03:35     INFO - TEST-INFO | screentopng: exit 0
[task 2017-03-22T19:03:35.482190Z] 19:03:35     INFO - Buffered messages logged at 19:03:34
[task 2017-03-22T19:03:35.485006Z] 19:03:35     INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | downloadLastDir should be a valid object - 
[task 2017-03-22T19:03:35.489792Z] 19:03:35     INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | LastDir pref should be null to start with - 
[task 2017-03-22T19:03:35.494381Z] 19:03:35     INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | LastDir should point to the tmpDir - 
[task 2017-03-22T19:03:35.496805Z] 19:03:35     INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | downloadLastDir.file should not be pointing to tmpDir - 
[task 2017-03-22T19:03:35.498808Z] 19:03:35     INFO - Buffered messages finished
[task 2017-03-22T19:03:35.506482Z] 19:03:35     INFO - TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | {} - 
[task 2017-03-22T19:03:35.509501Z] 19:03:35     INFO - Stack trace:
[task 2017-03-22T19:03:35.512705Z] 19:03:35     INFO - chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js:test/<:10
[task 2017-03-22T19:04:17.210898Z] 19:04:17     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2017-03-22T19:05:02.212758Z] 19:05:02     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-03-22T19:05:02.216642Z] 19:05:02     INFO - TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_DownloadLastDirWithCPS.js | Test timed out - 

There are more failures, please verify with Try that these also got fixed. Thank you for your endurance.

Flags: needinfo?(ehsan)

Ryan VanderMeulen [:RyanVM]

Assignee

Comment 25

•

7 years ago

So this completely blew up on inbound.  Again.  :(

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ef22dba0dac560980462f670db732470d5e364c1

For one thing we have this test which implements nsILoadContext.  <http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/netwerk/test/unit/test_bug826063.js#22>  As the reviewer of the test, I'd be fine with removing it at this point.  What do you think, Andrea?

Also, I'm not 100% sure if making this method void was the right idea.  I suspect some of these bustages are the result of those changes (I admit I didn't push to try after addressing those changes.)  See the places in the patch where I'm removing the checks for the return values.  Can you please try to convince me why it's OK to make this method return void?

Flags: needinfo?(amarchesini)

Updated

•

7 years ago

status-firefox52: affected → wontfix

status-firefox53: affected → wontfix

status-firefox54: --- → wontfix

status-firefox55: --- → affected

Keywords: regression

Jason Duell

Comment 27

•

7 years ago

re: comment 25:  I assume the issue is that we don't want to ever have a JS-implemented nsILoadContext?

I looked and we've got 3 necko unit tests that implement nsILoadContext:  test_predictor.js, test_cacheflags.js, and test_bug826063.js.  Test_predictor is just implementing it because predictor.predict() takes one as an argument (does passing 'null' in JS result in null in the C++ code? If so we could just change the arg to null and the predictor C++ code will skip the check, which is the same as privateBrowsing=false, so no change to test logic/result).  Test_cacheflags is using it to make sure we don't store into the cache during PB.  That would be nice to have test coverage for, but if it's a pain to support it we can live w/o it.

So yes, we can blow away all these test implementations of nsILoadContext as needed.

Jason Duell

Comment 28

•

7 years ago

Nick, can you write a patch that changes the tests mentioned in comment 27 (sounds like test_bug826063.js will just go away? Remove refs to nsILoadContext from the other two), push it to try and see if anything still breaks? I'm trying to take some load off ehsan.

Flags: needinfo?(hurley)

Comment 29

•

7 years ago

test_predictor doesn't create an nsILoadContext (I think it did in the past, but the change to use originAttributes got rid of that), so that one's fine. Seems like everyone's ok getting rid of test_bug826063, so that's easy.

However, I worry about removing our test for not storing into the cache during PB. That seems like the kind of thing we would want to have tested. Like, I don't *think* we'd break it, but given we're all human... I don't trust us that far :)

Would it be totally crazy to have some way to get a natively-implemented nsILoadContext that we could use in these tests? That would make me feel more comfortable than just dropping our testing of caching in PB mode.

Flags: needinfo?(hurley) → needinfo?(jduell.mcbugs)

Jason Duell

Comment 30

•

7 years ago

I'd be fine with some kind of dummy nsLoadContext.cpp that our tests could grab from XPCOM somehow (and be able to set the privateBrowsing field).  That class is mostly a stub anyway.  But going back to comment 0, it looks like the existing code is calling GetOriginAttributes() on the loadContext, and I'm not sure what we'd hand back.  We may need to sync with ehsan to figure out what to do with this.

(meanwhile nick is going to do a try run with all the JS test nsILoadContexts removed and see what the results look like.  Nick, you could also try making GetOriginAttributes not return void...).

Flags: needinfo?(jduell.mcbugs)

Assignee

Comment 31

•

7 years ago

Comment on attachment 8848792 [details] [diff] [review]
Avoid going into SpiderMonkey for retrieving origin attributes

Sorry for my delay here, the test issue I think is fairly easy to solve, we should be able to add an XPCOM constructor to the class in docshell/base/LoadContext.h and just use that in the test.  I don't see any reason why that wouldn't work.

But I'm still waiting for baku's response to the rest of comment 25.  Let me try resetting the review request.  :-)

Flags: needinfo?(ehsan)

Attachment #8848792 - Flags: review+ → review?(amarchesini)

Comment 32

•

7 years ago

Comment on attachment 8848792 [details] [diff] [review]
Avoid going into SpiderMonkey for retrieving origin attributes

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

Sorry for the delay:

1. OK with removing that test.
2. I still think that we don't need to return a boolean. Each GetOriginAttributes() returns true (If you change TabParent). so, so far, there are no needs to return false and or to check the return value.

Attachment #8848792 - Flags: review?(amarchesini) → review+

Comment hidden (mozreview-request)

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

Comment 34

•

7 years ago

The patch changes the tests that create an nsILoadContext in script to use ones made natively. It's meant to apply on top of Ehsan's patch from above. Running through try now https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bd6d6b8b9ab84257ccd33e223033809a6177387 with both patches applied (though in the opposite order, 'cause I'm silly).

Assignee

Comment 35

•

7 years ago

mozreview-review

Comment on attachment 8855502 [details]
Bug 1316683 - make nsILoadContext non-scriptable.

https://reviewboard.mozilla.org/r/127352/#review130134

Thank you!

::: netwerk/test/unit/test_bug826063.js:74
(Diff revision 1)
>   * Load context mandates private mode
>   */
>  add_test(function test_LoadContextPrivate() {
> -  let ctx = new LoadContext(true);
>    for (let c of getChannels()) {
> -    c.notificationCallbacks = ctx;
> +    c.notificationCallbacks = privateContext;

This is changing the test slightly by reusing the same load context object.  I suggest changing the above to something like:

let LoadContext = Components.Constructor("@mozilla.org/loadcontext;1");
// similarly for PrivateLoadContext

and just change the definition of ctx here and below accordingly.

Attachment #8855502 - Flags: review?(ehsan) → review+

Assignee

Comment 36

•

7 years ago

(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #34)
> The patch changes the tests that create an nsILoadContext in script to use
> ones made natively. It's meant to apply on top of Ehsan's patch from above.
> Running through try now
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3bd6d6b8b9ab84257ccd33e223033809a6177387 with both
> patches applied (though in the opposite order, 'cause I'm silly).

There are still test failures.  I need to look into why.

Assignee

Comment 37

•

7 years ago

Attached patch Part 2: Use the non-scriptable LoadContext object in the contentprefs module — Details — Splinter Review

Attachment #8856259 - Flags: review?(hurley)

Assignee

Comment 38

•

7 years ago

Attached patch Part 3: Use the non-scriptable LoadContext object in DownloadLastDir.jsm — Details — Splinter Review

Attachment #8856267 - Flags: review?(hurley)

Assignee

Comment 39

•

7 years ago

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #35)
> Comment on attachment 8855502 [details]
> Bug 1316683 - make nsILoadContext non-scriptable.
> 
> https://reviewboard.mozilla.org/r/127352/#review130134
> 
> Thank you!
> 
> ::: netwerk/test/unit/test_bug826063.js:74
> (Diff revision 1)
> >   * Load context mandates private mode
> >   */
> >  add_test(function test_LoadContextPrivate() {
> > -  let ctx = new LoadContext(true);
> >    for (let c of getChannels()) {
> > -    c.notificationCallbacks = ctx;
> > +    c.notificationCallbacks = privateContext;
> 
> This is changing the test slightly by reusing the same load context object. 
> I suggest changing the above to something like:
> 
> let LoadContext = Components.Constructor("@mozilla.org/loadcontext;1");
> // similarly for PrivateLoadContext
> 
> and just change the definition of ctx here and below accordingly.

In order to simplify the landing process, I will address this comment myself when landing.  :-)

Assignee

Comment 40

•

7 years ago

Attached patch Part 4: Use the non-scriptable LoadContext object in test_private_channel.js — Details — Splinter Review

Attachment #8856289 - Flags: review?(hurley)

Assignee

Comment 41

•

7 years ago

This approach of declaring a pure virtual function doesn't work on Windows: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f93705dad773fa993d3d54324b2f977b3bdf8c2>  For some reason, the xpt caller ends up calling that function, presumably the ordering of overloads is different.  Let's move things to XPIDL and not rely on compiler magic!

Flags: needinfo?(amarchesini)

Assignee

Comment 42

•

7 years ago

Attached patch Part 5: Avoid going into SpiderMonkey for retrieving origin attributes — Details — Splinter Review

This is the latest version version of the patch, and I think it has changed
sufficiently to merit another review.  Sorry for the additional requests on
this bug.

Attachment #8856323 - Flags: review?(amarchesini)

Assignee

Updated

•

7 years ago

Attachment #8848792 - Attachment is obsolete: true

Assignee

Comment 43

•

7 years ago

Green on try: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=79087ca8bf33b24f13dab94c045757f93d14860b>

Updated

•

7 years ago

Attachment #8856259 - Flags: review?(hurley) → review+

Updated

•

7 years ago

Attachment #8856267 - Flags: review?(hurley) → review+

Updated

•

7 years ago

Attachment #8856289 - Flags: review?(hurley) → review+

Updated

•

7 years ago

Attachment #8856323 - Flags: review?(amarchesini) → review+