Bug 104472 – [Windows]ASSERTION failed in Windows: css3/css3-modsel-33.html

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[huangxueqing]

In WebFrameLoaderClient::dispatchDidLayout, |milestones| maybe was |DidFirstLayout & DidFirstVisuallyNonEmptyLayout|, which will call webView->frameLoadDelegatePrivate(&frameLoadDelegatePrivate) twice, we should rest |frameLoadDelegatePrivate| after the first one, otherwise |T** operator&()| will hit |ASSERT(!m_ptr)|.

[huangxueqing]

Created attachment 178402 [details]
patch

[Brent Fulgham]

Comment on attachment 178402 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review

I think this looks like a good change, but I wonder if things might be clearer if we didn't reuse the variable for two different purposes.

> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429
> +    frameLoadDelegatePrivate = 0;

This seems like a good change, but would it be clearer if we used separate COMPtrs for the two possible cases? Maybe we should have each one declared within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout?

[Brent Fulgham]

Tim, what do you think about making these two separate variables, rather than reusing the same COMPtr?

[Tim Horton]

Comment on attachment 178402 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review

>> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429
>> +    frameLoadDelegatePrivate = 0;
> 
> This seems like a good change, but would it be clearer if we used separate COMPtrs for the two possible cases? Maybe we should have each one declared within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout?

I agree, Brent's plan (two separate locals inside the if blocks) is clearer.

[Brent Fulgham]

Comment on attachment 178402 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review

Thanks for taking the time to post this fix.  I think it looks good, but we would like you to move the COMPtr declaration inside the two "if" cases. This will make it clearer that the COMPtr is not needed elsewhere.
For this reason, r-, but please update the patch and I'll approve it.

>>> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429
>>> +    frameLoadDelegatePrivate = 0;
>> 
>> This seems like a good change, but would it be clearer if we used separate COMPtrs for the two possible cases? Maybe we should have each one declared within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout?
> 
> I agree, Brent's plan (two separate locals inside the if blocks) is clearer.

Okay -- please move the scope of "COMPtr<IWebFrameLoadDelegatePrivate> frameLoadDelegatePrivate" to be enclosed inside the two cases.

[huangxueqing]

Created attachment 178695 [details]
patch

[huangxueqing]

(In reply to comment #5)
> (From update of attachment 178402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review
> 
> Thanks for taking the time to post this fix.  I think it looks good, but we would like you to move the COMPtr declaration inside the two "if" cases. This will make it clearer that the COMPtr is not needed elsewhere.
> For this reason, r-, but please update the patch and I'll approve it.
> 
> >>> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429
> >>> +    frameLoadDelegatePrivate = 0;
> >> 
> >> This seems like a good change, but would it be clearer if we used separate COMPtrs for the two possible cases? Maybe we should have each one declared within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout?
> > 
> > I agree, Brent's plan (two separate locals inside the if blocks) is clearer.
> 
> Okay -- please move the scope of "COMPtr<IWebFrameLoadDelegatePrivate> frameLoadDelegatePrivate" to be enclosed inside the two cases.

Done, it seems more reasonable, thanks for review.

[WebKit Review Bot]

Comment on attachment 178695 [details]
patch

Rejecting attachment 178695 [details] from commit-queue.

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Full output: http://queues.webkit.org/results/15260214

[WebKit Review Bot]

Comment on attachment 178695 [details]
patch

Clearing flags on attachment: 178695

Committed r137246: <http://trac.webkit.org/changeset/137246>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.