Closed Bug 1316683 Opened 3 years ago Closed 3 years ago

mozilla::net::PredictorLearn() uses JS during painting

Categories

(Core :: Networking, defect, critical)

51 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: mccr8, Assigned: ehsan)

References

Details

(Keywords: crash, Whiteboard: [necko-next])

Crash Data

Attachments

(5 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-68d18789-93f6-4eeb-a830-5be2f2161109.
=============================================================

mozilla::dom::FontFaceSet::StartLoad() can call mozilla::net::PredictorLearn(). This in turn can call IPC::SerializedLoadContext::Init(), I think due to bug Bug 1304219. This ends up calling nsILoadContext::GetOriginAttributes, which ends up in the JS engine. If this happens during painting, then we hit a fatal assert.
I don't think this is related to the Bug 1304219 since it will call nsILoadContext::GetOriginAttributes() even before the Bug 1304219.
Nick, do you have time to take a look?
Flags: needinfo?(hurley)
probably someone from the security group is a better person to figure out originAttributes.
Whiteboard: [necko-next]
Sorry, that got mangled. The crash under this circumstance goes back as far as 51 (from bug 1279086 and deps), so calling this a regression from that.
Blocks: 1279086
Flags: needinfo?(tihuang)
Maybe baku knows about the originAttributes aspect here and can help with some insight?
Flags: needinfo?(amarchesini)
I think this can be closed now. It no longer applies after bug 1328423.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(hurley)
Resolution: --- → WONTFIX
Flags: needinfo?(amarchesini)
I'd like to try to revive the paint during GC project.  :-)  Taking this one.
Assignee: nobody → ehsan
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(In reply to :Ehsan Akhgari from comment #8)
> I'd like to try to revive the paint during GC project.  :-)  Taking this one.

Why?
(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.)
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)
Blocks: 1348460
Duplicate of this bug: 1348463
(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 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+
(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. :-)
(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.
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fecc6abcf708
Avoid going into SpiderMonkey for retrieving origin attributes; r=baku
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)
Sorry, my local build was too slow, and I was too impatient.  :(
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e47807067a6
Avoid going into SpiderMonkey for retrieving origin attributes; r=baku
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)
So sorry.  :(
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef22dba0dac5
Avoid going into SpiderMonkey for retrieving origin attributes; r=baku
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)
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)
Duplicate of this bug: 1348460
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.
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)
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)
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)
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 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+
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).
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+
(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.
(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.  :-)
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)
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)
Attachment #8848792 - Attachment is obsolete: true
Attachment #8856259 - Flags: review?(hurley) → review+
Attachment #8856267 - Flags: review?(hurley) → review+
Attachment #8856289 - Flags: review?(hurley) → review+
Attachment #8856323 - Flags: review?(amarchesini) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd78de6136de
Part 1: Make nsILoadContext non-scriptable. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/1542d591be9f
Part 2: Use the non-scriptable LoadContext object in the contentprefs module; r=hurley
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2688db7d9d
Part 3: Use the non-scriptable LoadContext object in DownloadLastDir.jsm; r=hurley
https://hg.mozilla.org/integration/mozilla-inbound/rev/448f478547de
Part 4: Use the non-scriptable LoadContext object in test_private_channel.js; r=hurley
https://hg.mozilla.org/integration/mozilla-inbound/rev/adf1dbdf0042
Part 5: Avoid going into SpiderMonkey for retrieving origin attributes; r=baku
You need to log in before you can comment on or make changes to this bug.