Opened 7 years ago

Closed 6 years ago

#8167 closed defect (fixed)

Replace deprecated getOriginatingURI() method in Torbutton

Reported by: gk Owned by: mikeperry
Priority: Medium Milestone:
Component: TorBrowserButton Version:
Severity: Keywords: tbb-linkability, ff24-esr, MikePerry201312R
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Mozilla removed getOriginatingURI() in Gecko18 (See: https://bugzilla.mozilla.org/show_bug.cgi?id=769589). It needs to get replaced to fix corner cases like the one in #3739.

Child Tickets

Attachments (4)

0001-replacing-getOriginatingURI.patch (3.2 KB) - added by gk 7 years ago.
0002-Bug-8167-replacing-the-obsolete-getOriginatingURI-me.patch (4.1 KB) - added by gk 6 years ago.
SafeCache-GetFirstPartyUri.diff (2.6 KB) - added by mikeperry 6 years ago.
Patch using getFirstPartyURI (fails with NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)
SafeCache-getFirstPartyURIFromChannel.diff (3.4 KB) - added by mcs 6 years ago.
SafeCache patch that uses getFirstPartyURIFromChannel()

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by gk

comment:1 Changed 7 years ago by gk

Status: newneeds_review

Mozilla folks made me believe that there is actually no need for getOriginatingURI() at all. And it turns out that #3739 was fixable using the notificationCallbacks alone. I've implemented that into JonDoFox and the attached patch contains the fix for Torbutton.

comment:2 Changed 7 years ago by mikeperry

Keywords: tbb-linkability ff24-esr added

comment:3 Changed 6 years ago by gk

Status: needs_reviewneeds_revision

The patch contains bugs and does not apply cleanly anymore.

comment:4 Changed 6 years ago by gk

Keywords: MikePerry201311R added
Status: needs_revisionneeds_review

This should be fixed now. I tested it successfully with the example given in #3739. Setting the magic keyword assuming Mike wants to have this as well on his November radar...

comment:5 Changed 6 years ago by mikeperry

Hrmm.. I think I feel more comfortable using our API for this. We forward-ported the missing pieces from FF17 so that the getFirstPartyURI would continue to function. I'd rather do that for now, and use that work towards a C++ API that Mozilla was comfortable with.

Sorry I didn't say this sooner :/. Let me know if you think otherwise, of course.

comment:6 Changed 6 years ago by gk

Status: needs_reviewneeds_revision

No, that's even better than the current solution. I'll update the patch shortly.

Last edited 6 years ago by gk (previous) (diff)

comment:7 Changed 6 years ago by mikeperry

Keywords: MikePerry201312R added; MikePerry201311R removed

Changed 6 years ago by mikeperry

Patch using getFirstPartyURI (fails with NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)

comment:8 Changed 6 years ago by mikeperry

Cc: mcs brade added

For the record: It turns out that using getFirstPartyURI fails with a weird exception here (NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO). I decided to update this bug with a modified version of the patch anyway in case either mcs or brade have any ideas why this API would fail here. As far as I can tell, it's throwing that exception before the C++ is even invoked :/.

comment:9 in reply to:  8 Changed 6 years ago by mcs

Replying to mikeperry:

For the record: It turns out that using getFirstPartyURI fails with a weird exception here (NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO). I decided to update this bug with a modified version of the patch anyway in case either mcs or brade have any ideas why this API would fail here. As far as I can tell, it's throwing that exception before the C++ is even invoked :/.

It turns out that the problem is that nsIDocument is not IDL'd, so we cannot use it from JS. Even though we just want to pass null for the second parameter to getFirstPartyURI(), things blow up when the browser tries to get type info for that parameter. Kathy says that the reason nsIDocument is not IDL'd is to improve performance.

We have a couple of options:

Option 1: In the IDL, put [noscript] in front of the existing getFirstPartyURI() method and add a second method that just takes a channel, e.g.,

[noscript] nsIURI getFirstPartyURI(in nsIChannel aChannel, in nsIDocument aDoc);
...
nsIURI getFirstPartyURIFromChannel(in nsIChannel aChannel)

Existing C++ code would not need to be changed, and we would call the new method from the SafeCache code inside Torbutton. This is probably the easiest solution.

Option 2: Change the type of the aDoc parameter to nsISupports and QI it inside the C++ implementation, e.g.,

nsIURI getFirstPartyURI(in nsIChannel aChannel, in nsISupports aDocSupports);

This probably would force us to change all of our existing C++ calls to getFirstPartyURI() and we would lose some type safety in the API.

comment:10 Changed 6 years ago by mcs

Also -- after Kathy and I hacked things to basically implement option 1 we get an "illegal value" exception on some calls to getFirstPartyURI() from within the SafeCache code, e.g.,

[12-04 20:22:11] Torbutton NOTE: FirstParty API failed to get parent: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [mozIThirdPartyUtil.getFirstPartyURIFromChannel]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://torbutton/content/stanford-safecache.js :: SSC_RequestListener.prototype.onModifyRequest :: line 162" data: no] http://gtglobal-ocsp.geotrust.com/

We have not debugged that problem yet... or maybe it is expected. I think all of the errors we have seen are OCSP or some other behind-the-scenes load (which means there is no first party URI).

comment:11 Changed 6 years ago by mikeperry

Yes, for now option 1 seems fine. Later I suppose we should investigate if other forms of document can be used instead of nsIDocument. For instance, nsIDOMDocument is IDLd and may be usable? nsDocument implements that interface.

As for the OCSP exceptions, that failure is OK. Not sure the best way to handle it and other non-content window loads in the API (and its usage) wrt error console logging though..

comment:12 in reply to:  11 Changed 6 years ago by mcs

Replying to mikeperry:

As for the OCSP exceptions, that failure is OK. Not sure the best way to handle it and other non-content window loads in the API (and its usage) wrt error console logging though..

How about this:

nsIURI getFirstPartyURIFromChannel(in nsIChannel aChannel, in bool aLogErrors);

It might be nice to add an aLogErrors parameter to the existing getFirstPartyURI() method, but that can be done some other time.

comment:13 Changed 6 years ago by mikeperry

Sounds like a plan. Still not sure how we'll decide when we should log in Torbutton, but for now we can pass false into the API, and safelog the exception that's thrown at a lower loglevel.

In the future, we probably want to move all of this cache isolation into C++ anyway, and/or leverage the new private browsing API to isolate cache instead.

comment:14 Changed 6 years ago by mcs

We committed the browser fix:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/e7fc375a4b2e4eb3dec47b4c17358c6c7c82fdf7

I will also attach to this bug the Torbutton patch we used during testing, but will leave that to Mike to tweak and commit.

Changed 6 years ago by mcs

SafeCache patch that uses getFirstPartyURIFromChannel()

comment:15 Changed 6 years ago by mikeperry

Resolution: fixed
Status: needs_revisionclosed

Ok, thanks! This is now merged, and should appear in the first TBB3.5 release. I created #10319 for the cache isolation patch cleanup work.

Note: See TracTickets for help on using tickets.