Chromium Code Reviews
Help | Chromium Project | Sign in
(8)

Issue 12395021: Fix a crash seen in ChromeFrame when opening a non CF top level tab from a CF page. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by ananta
Modified:
2 years, 1 month ago
Reviewers:
robertshield
CC:
chromium-reviews, grt+watch_chromium.org, amit, robertshield
Visibility:
Public.

Description

Fix a crash seen in ChromeFrame when opening a non CF top level tab from a CF page. This was a regression caused by the fix for bug http://code.google.com/p/chromium/issues/detail?id=168308 which was to not invalidate the protocol sink mapping for CF pages which are switched in from IE. The crash in this case occurs because the protocol sink mapping is removed in the call to IInternetProtocol::Terminate as this is treated as a ChromeFrame request. This is only a temporary CF url which eventually transitions to the actual URL which is opened in IE. For the curious we intercept IInternetProtocol::LockRequest and for the special gcf://attach_external_tab requests we addref the protocol data and return without calling the original LockRequest API. When UnlockRequest is invoked we rely on the protocol data mapping to exist to release the protocol data and return without calling the original UnlockRequest API. In this case the sequence is IInternetProtocol::LockRequest, IInternetProtocolRoot::Terminate followed by IInternetProtocol::UnlockRequest. In our terminate handler we remove the protocol data mapping for the attach tab request. When UnlockRequest is called we call the original API and end up crashing. Fix is to not invalidate the protocol data mapping for attach tab requests. A test for this is in the works by robertshield. BUG=178415 R=robertshield Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185956

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -3 lines) Patch
M chrome_frame/protocol_sink_wrap.cc View 1 1 chunk +16 lines, -3 lines 1 comment Download
Trybot results:
Commit: CQ not working?

Messages

Total messages: 4 (0 generated)
ananta
2 years, 1 month ago (2013-03-04 19:19:57 UTC) #1
robertshield
Change LGTM, but with a request for a detailed comment. https://chromiumcodereview.appspot.com/12395021/diff/1/chrome_frame/protocol_sink_wrap.cc File chrome_frame/protocol_sink_wrap.cc (right): https://chromiumcodereview.appspot.com/12395021/diff/1/chrome_frame/protocol_sink_wrap.cc#newcode874 ...
2 years, 1 month ago (2013-03-04 19:40:38 UTC) #2
ananta
https://chromiumcodereview.appspot.com/12395021/diff/1/chrome_frame/protocol_sink_wrap.cc File chrome_frame/protocol_sink_wrap.cc (right): https://chromiumcodereview.appspot.com/12395021/diff/1/chrome_frame/protocol_sink_wrap.cc#newcode874 chrome_frame/protocol_sink_wrap.cc:874: if (prot_data && !IsChrome(prot_data->renderer_type()) && On 2013/03/04 19:40:39, robertshield ...
2 years, 1 month ago (2013-03-04 19:52:49 UTC) #3
robertshield
2 years, 1 month ago (2013-03-04 19:56:13 UTC) #4
https://chromiumcodereview.appspot.com/12395021/diff/5002/chrome_frame/protoc...
File chrome_frame/protocol_sink_wrap.cc (right):

https://chromiumcodereview.appspot.com/12395021/diff/5002/chrome_frame/protoc...
chrome_frame/protocol_sink_wrap.cc:887: //    before UnlockRequest causes IE to
crash.
Awesome comment, thanks!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 700cc9d