Closed Bug 829043 Opened 7 years ago Closed 7 years ago

wyciwyg cache should use app jars

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: jdm, Assigned: jdm)

Details

Attachments

(1 file, 5 obsolete files)

There's currently a global wyciwyg:// cache. I suspect that two apps displaying a page that does document.write could end up sharing content, which would be bad.
blocking-basecamp: ? → +
Assignee: nobody → josh
Attachment #700454 - Flags: review?(michal.novotny)
Sorry, forgot to qref.
Attachment #700465 - Flags: review?(michal.novotny)
Attachment #700454 - Attachment is obsolete: true
Attachment #700454 - Flags: review?(michal.novotny)
Michal says he can review this in 2-3h.
Comment on attachment 700465 [details] [diff] [review]
Separate wyciwyg cache into app jars.

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

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +692,5 @@
>    else
>      storagePolicy = nsICache::STORE_ANYWHERE;
>  
> +  nsCOMPtr<nsILoadContext> loadContext;
> +  NS_QueryNotificationCallbacks(this, loadContext);

Have you tested this?  We don't currently send TabBrowser/SerializedLoadContext in PNecko.ipdl for wyciwyg channels, so we won't have a valid loadcontext here.  For this (and security) I suspect we need to start passing those and do the usual Necko IPDL security checks.  If so write it up as a separate patch and you can have my +r if it's as straightforward a patch as I suspect it should be.
Awkward.
Attachment #700510 - Flags: review?(jduell.mcbugs)
Comment on attachment 700510 [details] [diff] [review]
Hook up load contexts for remote wyciwyg and ftp channels.

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

Better late than never :)
Attachment #700510 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 700465 [details] [diff] [review]
Separate wyciwyg cache into app jars.

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

Rest of patch looks good to me, with one nit.

Michal, I'm ok with +r'ing this unless you want to also look at it.

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +698,5 @@
> +  bool isInBrowser = false;
> +  if (loadContext) {
> +    loadContext->GetAppId(&appId);
> +    loadContext->GetIsInBrowserElement(&isInBrowser);
> +  }

Use NS_GetAppInfo here instead of getting the fields by hand.  Shorter and more robust in the face of appinfo changes (we'll break all the call sites if we add a field, etc).
Attachment #700465 - Flags: feedback+
Comment on attachment 700465 [details] [diff] [review]
Separate wyciwyg cache into app jars.

Looks good. Just please fix the indentation in nsWyciwygProtocolHandler::Observe() before the push. There are 4 spaces instead of 2.
Attachment #700465 - Flags: review?(michal.novotny) → review+
Attachment #700465 - Attachment is obsolete: true
Attachment #700510 - Attachment is obsolete: true
Attachment #700804 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=0b2f27264bec

Qref, my old friend. We meet again.
Attachment #700978 - Flags: review?(jduell.mcbugs)
Attachment #700806 - Attachment is obsolete: true
Comment on attachment 700978 [details] [diff] [review]
Separate wyciwyg cache into app jars.

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

Looks good but we can rip a fair amount of code out.  See comments.  +r if you agree with them all.

::: netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp
@@ +569,5 @@
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
> +static mozilla::dom::TabChild*
> +GetTabChild(nsIChannel* aChannel)

This is nice.  We should add this to nsNetUtil.h (or the nsNetInternalUtil.h that I keep meaning to create) and have the other necko channel types use it.  Not today :)

@@ +620,5 @@
>  {
>    NS_ENSURE_TRUE((mState == WCC_INIT) ||
>                   (mState == WCC_ONWRITE), NS_ERROR_UNEXPECTED);
>  
> +  if (!mSentAppData) {

Did you mean to set mSentAppData = true somewhere in AsyncOpen?  If we've called SendAsyncOpen we can set it IIUC.

::: netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp
@@ +40,5 @@
>    // yet, but we must not send any more msgs to child.
>    mIPCClosed = true;
> +
> +  // We need to force the cycle to break here
> +  mChannel->SetNotificationCallbacks(nullptr);

I assume this was cause of memory leaks?  Any chance we need to clear callbacks in FTP::ActorDestroy too?  Let's do that, just in case :)

@@ +95,5 @@
> +
> +  if (!SetupAppData(loadContext, parent))
> +    return false;
> +
> +  mChannel->SetNotificationCallbacks(this);

might as well roll the SetNotificationCallbacks call into SetupAppData.

@@ +161,5 @@
> +  rv = mChannel->SetNotificationCallbacks(this);
> +  if (NS_FAILED(rv))
> +    return SendCancelEarly(rv);
> +
> +  if (compareAppData) {

IPDL is supposed to guarantee that the appID/mBrowser is the same.  If that's not true we'd be seriously hosed.  So let's just get rid of all this compareAppData logic.

@@ +189,5 @@
>  bool
>  WyciwygChannelParent::RecvWriteToCacheEntry(const nsString& data)
>  {
> +  if (!mReceivedAppData) {
> +    printf_stderr("WyciwygChannelParent::RecvWriteToCacheEntry: didn't receive app data\n");

Put FATAL ERROR and/or KILLING CHILD in error msg, so they stick out in adb logcat

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +697,5 @@
>      storagePolicy = nsICache::STORE_ANYWHERE;
>  
>    nsCOMPtr<nsICacheSession> cacheSession;
> +  nsAutoCString sessionName;
> +  nsWyciwygProtocolHandler::GetCacheSessionName(mAppId, mInBrowser,

I'd prefer that we just call NS_GetAppInfo right before calling GetCacheSession, with temp vars, and just get rid of mAppId/mInBrowser.  Unless there's some benefit to having member variables that I don't understand.
Attachment #700978 - Flags: review?(jduell.mcbugs) → review+
(In reply to Jason Duell (:jduell) from comment #17)
> @@ +620,5 @@
> >  {
> >    NS_ENSURE_TRUE((mState == WCC_INIT) ||
> >                   (mState == WCC_ONWRITE), NS_ERROR_UNEXPECTED);
> >  
> > +  if (!mSentAppData) {
> 
> Did you mean to set mSentAppData = true somewhere in AsyncOpen?  If we've
> called SendAsyncOpen we can set it IIUC.

Agreed.
 
> ::: netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp
> @@ +40,5 @@
> >    // yet, but we must not send any more msgs to child.
> >    mIPCClosed = true;
> > +
> > +  // We need to force the cycle to break here
> > +  mChannel->SetNotificationCallbacks(nullptr);
> 
> I assume this was cause of memory leaks?  Any chance we need to clear
> callbacks in FTP::ActorDestroy too?  Let's do that, just in case :)

We don't. nsFTPChannel clears its callbacks in OnStopRequest, and the code will always flow through there. nsWyciwygChannel is different because of the WriteToCacheEntry usage, where you may never go through OnStopRequest.
 
> @@ +95,5 @@
> > +
> > +  if (!SetupAppData(loadContext, parent))
> > +    return false;
> > +
> > +  mChannel->SetNotificationCallbacks(this);
> 
> might as well roll the SetNotificationCallbacks call into SetupAppData.

Nope, since failure is handled differently in RecvAsyncOpen.

> @@ +161,5 @@
> > +  rv = mChannel->SetNotificationCallbacks(this);
> > +  if (NS_FAILED(rv))
> > +    return SendCancelEarly(rv);
> > +
> > +  if (compareAppData) {
> 
> IPDL is supposed to guarantee that the appID/mBrowser is the same.  If
> that's not true we'd be seriously hosed.  So let's just get rid of all this
> compareAppData logic.

I'd like to keep this to ensure that the data sent along with AsyncOpen does not contradict any data received earlier via WriteToCacheEntry. Then again, CloseCacheEntry triggers IPDL actor deletion, so perhaps I could just assert that we haven't seen any app data before. However, I am not familiar enough with wyciwyg to feel very comfortable making that assertion this close to shipping.

> @@ +189,5 @@
> >  bool
> >  WyciwygChannelParent::RecvWriteToCacheEntry(const nsString& data)
> >  {
> > +  if (!mReceivedAppData) {
> > +    printf_stderr("WyciwygChannelParent::RecvWriteToCacheEntry: didn't receive app data\n");
> 
> Put FATAL ERROR and/or KILLING CHILD in error msg, so they stick out in adb
> logcat

Ok.

> ::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
> @@ +697,5 @@
> >      storagePolicy = nsICache::STORE_ANYWHERE;
> >  
> >    nsCOMPtr<nsICacheSession> cacheSession;
> > +  nsAutoCString sessionName;
> > +  nsWyciwygProtocolHandler::GetCacheSessionName(mAppId, mInBrowser,
> 
> I'd prefer that we just call NS_GetAppInfo right before calling
> GetCacheSession, with temp vars, and just get rid of mAppId/mInBrowser. 
> Unless there's some benefit to having member variables that I don't
> understand.

Can't. OpenCacheEntry can be called off the main thread, and attempting to use the loadgroup will make Gecko scream bloody murder.
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.