Last Comment Bug 763352 - [SPDY] Accessing google homepage through proxy throws download prompt
: [SPDY] Accessing google homepage through proxy throws download prompt
Status: VERIFIED FIXED
[spdy][http-conn]
: regression
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 15 Branch
: x86 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Patrick McManus [:mcmanus]
:
:
Mentors:
: 763614 766037 766069 766070 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-10 15:45 PDT by Grant
Modified: 2012-06-28 17:19 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed


Attachments
Sample of "fastbutton" save with SPDY v3 (5.08 KB, text/plain)
2012-06-11 12:19 PDT, alex_mayorga
no flags Details
Sample of "fastbutton" save with SPDY v2 (5.27 KB, text/plain)
2012-06-11 12:19 PDT, alex_mayorga
no flags Details
Last "save" of google.com (27.44 KB, text/plain)
2012-06-11 12:22 PDT, alex_mayorga
no flags Details
patch 0 (1.13 KB, patch)
2012-06-19 05:21 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
patch 1 (1.52 KB, patch)
2012-06-20 14:48 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Grant 2012-06-10 15:45:06 PDT
Hi all,

First reported on the 8/6/12 builds (images included):

http://forums.mozillazine.org/viewtopic.php?f=23&t=2484269

Poster reported the error console saying this:

    rror: SyntaxError: illegal character
    Source File: https://apis.google.com/_/apps-static/_/js/gapi/googleapis_client,iframes_styles_bubble_internal/rt=j/ver=nPNdQPXPV58.en./sv=1/am=!tbK8W_8mwqaIodoNDQ/d=1/rs=AItRSTPka8wZmQg3IxPiQbaB0K911EvjbQ
    Line: 1
    Source Code:
    �

    Error: ReferenceError: iframes is not defined
    Source File: https://plusone.google.com/_/+1/fastbutton?bsv=p&url=http%3A%2F%2Fgrooveshark.com%2Falbum%2FExitos%2BCon%2BTradicion%2BSinaloense%2F4311578&size=medium&count=true&expandTo=bottom&annotation=none&hl=en-US&jsh=m%3B%2F_%2Fapps-static%2F_%2Fjs%2Fgapi%2F__features__%2Frt%3Dj%2Fver%3DnPNdQPXPV58.en.%2Fsv%3D1%2Fam%3D!tbK8W_8mwqaIodoNDQ%2Fd%3D1%2Frs%3DAItRSTPka8wZmQg3IxPiQbaB0K911EvjbQ#id=I1_1337865230908&parent=http%3A%2F%2Fgrooveshark.com&rpctoken=818362826&_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_resizeMe%2C_renderstart
    Line: 11

    Error: ReferenceError: gadgets is not defined
    Source File: https://plusone.google.com/_/apps-static/_/js/plusone/p1b,p1p/rt=j/ver=NjiN4Wmo0ZQ.en_US./sv=1/am=!6XIE-74lcjlJP2TtRQ/d=1/rs=AItRSTOigJYNDAsai_VK6ZuY1u5jp_dOgg
    Line: 119

    Error: SyntaxError: illegal character
    Source File: https://mts0.google.com/vt/pt?lyrs=m%40177000000&las=25.672473561951584%3B-100.33212661743164%3B25.726611809740938%3B-100.27204513549805&z=13&ptv=1&callback=_xdc_._0h35zl3c2
    Line: 1
    Source Code:
    �

    Error: no element found
    Source File: https://testpilot.mozillalabs.com/testcases/index.json
    Line: 1


Any ideas?
Comment 1 Grant 2012-06-10 15:53:43 PDT
More info - I only get this when its set up as homepage , but accessing Google after startup it loads fine. My other PC which doesn't use a proxy does not do this. Both have the same addons etc.
Comment 2 Matthias Versen [:Matti] 2012-06-10 17:40:33 PDT
Does it make a difference if you toggle network.http.spdy.enabled to false
Clearing the disk cache may be necessary
Comment 3 Grant 2012-06-10 18:18:16 PDT
Hi Matthias,

Disabling SPDY fixes this, so I guess its SPDY related?
Comment 4 Loic 2012-06-11 08:10:06 PDT
Maybe a regression after SPDY v3 landed (Bug 761775).
Comment 5 Patrick McManus [:mcmanus] 2012-06-11 11:27:43 PDT
grant, I'm out of the office until next week - so patience is appreciated.

can you turn on spdy, but turn off network.http.spdy.enabled.v3 ? That means you will use v2 of spdy and we can see if the issue is specifically the new v3 code.

thanks!
Comment 6 alex_mayorga 2012-06-11 12:14:01 PDT
*** Bug 763614 has been marked as a duplicate of this bug. ***
Comment 7 alex_mayorga 2012-06-11 12:19:14 PDT
Created attachment 631981 [details]
Sample of "fastbutton" save with SPDY v3

I'm the original reporter of this issue.

This is still happening at random for me.

Attached find the some samples of "fastbutton" saves with both SPDY v3 and v2.
Comment 8 alex_mayorga 2012-06-11 12:19:51 PDT
Created attachment 631983 [details]
Sample of "fastbutton" save with SPDY v2
Comment 9 alex_mayorga 2012-06-11 12:22:20 PDT
Created attachment 631984 [details]
Last "save" of google.com

This is the latest result of "Saving" google.com as a file.
Comment 10 Patrick McManus [:mcmanus] 2012-06-11 12:24:28 PDT
alex - so you are seeing that with both spdy 2 and spdy 3 (it wasn't clear to me what the attachments are meant to illustrate)?

please also confirm for me whether you are or are not you are using a proxy.

please describe what a "fastbutton" save is. I'm not familiar with that term.

thanks!
Comment 11 alex_mayorga 2012-06-11 12:34:02 PDT
Patrick,

I believe "fastbutton" are those G+ buttons plastered all over the web these days.

I got prompts to "Save file as..." for those with both SPDY 2:
"X-Firefox-Spdy: 2"

and SPDY 3:

"X-Firefox-Spdy: 3"

Nightly is behind a proxy here.

Do let me know anything I could collect to get this resolved.
Comment 12 Grant 2012-06-11 14:51:49 PDT
(In reply to Patrick McManus [:mcmanus] from comment #5)
> grant, I'm out of the office until next week - so patience is appreciated.
> 
> can you turn on spdy, but turn off network.http.spdy.enabled.v3 ? That means
> you will use v2 of spdy and we can see if the issue is specifically the new
> v3 code.
> 
> thanks!

Hi Patrick,

Same for me - disabling and resetting after disabling SPDY v3 doesn't fix this issue. Hope that this helps.
Comment 13 Patrick McManus [:mcmanus] 2012-06-12 17:57:46 PDT
just fyi I'm away on vacation this week - this will get focused attention the week of June 18
Comment 14 alex_mayorga 2012-06-18 08:19:21 PDT
Patrick,

hope you've had a great vacation =)

I first noticed this on 07 Jun 2012 and reported it to mozillaZine Nightly thread[1].

The problem is still present on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120618030531

I can reproduce the "Save file as..." prompts randomly:
 - loading pages with G+ buttons results on prompt to save "fastbutton" (see comment 7 and comment 8)
 - searching for a word on the search bar results on a prompt to save "search" 
 - loading google.com results on a prompt to save a file (see comment 9)

Do let me know if there's anything else I could provide to resolve this.
 
1| http://forums.mozillazine.org/viewtopic.php?p=12043397#p12043397
Comment 15 Grant 2012-06-18 20:00:51 PDT
I noticed when going to Google it will bring the download prompt up - but refreshing it will allow you to browse it, with SPDY still enabled (according to SPDY indicator).

so in short, it seems like first visit is only affected.
Comment 16 Patrick McManus [:mcmanus] 2012-06-19 05:21:14 PDT
Created attachment 634359 [details] [diff] [review]
patch 0

I'm posting this patch as a work-in-progress for your to try out.

Try run is at : https://tbpl.mozilla.org/?tree=Try&rev=afab794cca53 (you can get builds from there, when they're made)

I'm not r?ing this quite yet because I still see a save-as issue in my local tree and I haven't determined if it is related to this bug or not yet.

The problem fixed here is that the server has sent a settings (or ping, or go-away) frame before we realize the SSL/NPN handshake is complete. We use the handshake to determine we are running SPDY. So the server data gets interpreted as straight up HTTP/1 incorrectly. 

Two things make this happen at this time.. 1] normally we won't read any data until we have written a transaction out, but because the proxy requires a CONNECT transaction to setup the tunnel that isn't true in this case.. and 2] until recently the pipeline datastructure was a mediator while NPN was being negotiated - and the pipeline code won't call transaction->writesegments() for any transaction that has not yet been written out in full.
Comment 17 Ekanan Ketunuti 2012-06-19 06:25:15 PDT
*** Bug 766069 has been marked as a duplicate of this bug. ***
Comment 18 Patrick McManus [:mcmanus] 2012-06-19 09:31:50 PDT
Comment on attachment 634359 [details] [diff] [review]
patch 0

The other save-as issue I mentioned in comment 16 turned out to be unrelated (bug 766159).
Comment 19 Matthias Versen [:Matti] 2012-06-19 10:06:15 PDT
*** Bug 766070 has been marked as a duplicate of this bug. ***
Comment 20 dhaval thakar 2012-06-20 00:27:34 PDT
*** Bug 766037 has been marked as a duplicate of this bug. ***
Comment 21 Honza Bambas (:mayhemer) 2012-06-20 13:11:24 PDT
Comment on attachment 634359 [details] [diff] [review]
patch 0

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

r=honzab

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1366,5 @@
>      bool again = true;
>  
>      do {
> +        if (mCompletedProxyConnect && !mNPNComplete) {
> +            LOG(("NsHttpConnection::OnSocketReadable %p return due to complete "

Change the leading N to small-case.

Please add an explanatory comment like:

"We are preventing reads from the socket by the transaction since the protocol must first be determined by NPN negotiation.  When the target server speaks SPDY, getting here means it most probably has sent SETTINGS frame that the transaction recognizes as HTTP/0.9 response."

@@ +1371,5 @@
> +                 "proxy tunnel but incomplete NPN state\n", this));
> +            rv = NS_OK;
> +            break;
> +        }
> +        

The blank line has white spaces, please remove them.
Comment 22 Honza Bambas (:mayhemer) 2012-06-20 13:29:21 PDT
Comment on attachment 634359 [details] [diff] [review]
patch 0

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

Errr...

Taking back.

Now I recall and realize that I'm experiencing this bug w/o any proxy being set up (in Aurora with SPDYv3 enabled).  The condition should just be if (!mNPNComplete).
Comment 23 Honza Bambas (:mayhemer) 2012-06-20 13:38:06 PDT
Ok, spoke too fast, the condition should be if (!mProxyConnectInProgress && !mNPNComplete)
Comment 24 Patrick McManus [:mcmanus] 2012-06-20 13:41:39 PDT
(In reply to Honza Bambas (:mayhemer) from comment #23)
> Ok, spoke too fast, the condition should be if (!mProxyConnectInProgress &&
> !mNPNComplete)

that's fine. I'm a little surprised the read handler is active without the proxy being used, but your incantation is fine.
Comment 25 Patrick McManus [:mcmanus] 2012-06-20 14:48:42 PDT
Created attachment 635078 [details] [diff] [review]
patch 1
Comment 26 Honza Bambas (:mayhemer) 2012-06-20 15:58:50 PDT
Comment on attachment 635078 [details] [diff] [review]
patch 1

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

r=honzab

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1370,5 @@
> +            // from the socket until the results of NPN
> +            // negotiation are known (which is determined from the write path).
> +            // If the server speaks SPDY it is likely the readable data here is
> +            // a spdy settings frame and without NPN it would be misinterpreted
> +            // as HTTP/1

HTTP/0.9 ?
Comment 27 Patrick McManus [:mcmanus] 2012-06-20 18:43:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/99038d3c6360
Comment 28 Ed Morley [:emorley] 2012-06-21 04:03:24 PDT
https://hg.mozilla.org/mozilla-central/rev/99038d3c6360
Comment 29 Patrick McManus [:mcmanus] 2012-06-21 07:01:02 PDT
I'll late this bake on m-c until the weekend and then nom for aurora assuming it goes well.
Comment 30 alex_mayorga 2012-06-22 06:15:06 PDT
Patrick,

Thanks for your fix, I no longer get the "Save file as..." prompts on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120621053048
Comment 31 Patrick McManus [:mcmanus] 2012-06-26 07:18:07 PDT
Comment on attachment 635078 [details] [diff] [review]
patch 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 726366
User impact if declined: common save-as dialog when connecting to spdy service via an http proxy
Testing completed (on m-c, etc.): on m-c for 5 days 
Risk to taking this patch (and alternatives if risky): risk med-low. This is a very heavily exercised path so we would probably know quickly if a problem was caused. alternative would be to disable spdy proxies on FF15
String or UUID changes made by this patch:
Comment 32 Alex Keybl [:akeybl] 2012-06-26 10:06:02 PDT
Comment on attachment 635078 [details] [diff] [review]
patch 1

[Triage Comment]
Since we expect to hear if there is any major fallout here, approving for Aurora 15.
Comment 33 Patrick McManus [:mcmanus] 2012-06-28 17:18:45 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd3fd0baf45f

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