Bug 109576

Created attachment 187847 [details] Patch

This may be related to Bug 93214.

yup *** This bug has been marked as a duplicate of bug 93214 ***

(In reply to comment #1) > Created an attachment (id=187847) [details] > Patch (In reply to comment #3) > yup > > *** This bug has been marked as a duplicate of bug 93214 *** It looks like some of the changes in the patch fix further GTK+ bugs though. So perhaps this bug should stay open and address those.

(In reply to comment #4) > (In reply to comment #1) > > Created an attachment (id=187847) [details] [details] > > Patch > > (In reply to comment #3) > > yup > > > > *** This bug has been marked as a duplicate of bug 93214 *** > > It looks like some of the changes in the patch fix further GTK+ bugs though. So perhaps this bug should stay open and address those. I agree, it might be a dup, but this patch also includes the required changes for redirect-methods.html to work.

(In reply to comment #5) > I agree, it might be a dup, but this patch also includes the required changes for redirect-methods.html to work. Do you mind also commenting on the other bug? The checks here looks simpler, but I'm having trouble evaluating which version of the change is better.

my bad. oh, it looks like this may have the DRT fix that will let us uncomment the test from 93214 as well. However, as I said there, I don't think we should be changing shouldRedirectAsGET(); the logic in that function is correct as-is. The bug is in doRedirect(); shouldRedirectAsGET() is saying "you don't need to change the method from the previous request" but doRedirect *does* change the method (accidentally, as a result of initializing request from the wrong data).

Created attachment 187893 [details] Patch

Comment on attachment 187893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187893&action=review > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:-1005 > WebKitWebResource* webResource = webkit_web_view_get_resource(webView, identifierString.get()); > > - // A NULL WebResource means the load has been interrupted, and > - // replaced by another one while this resource was being loaded. > - if (!webResource) > - return; So is the the case that webkit_web_view_get_resource can never return a NULL WebKitWebResource? If so you should probably ASSERT here. > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:-1018 > - webkit_web_resource_init_with_core_resource(webResource, coreResource.get()); Not entirely sure why this is removed.

(In reply to comment #9) > (From update of attachment 187893 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187893&action=review > > > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:-1005 > > WebKitWebResource* webResource = webkit_web_view_get_resource(webView, identifierString.get()); > > > > - // A NULL WebResource means the load has been interrupted, and > > - // replaced by another one while this resource was being loaded. > > - if (!webResource) > > - return; > > So is the the case that webkit_web_view_get_resource can never return a NULL WebKitWebResource? If so you should probably ASSERT here. The implementation can return NULL, but since we register all the identifiers and WebCore will never notify us about a non existing resource, I think we can safely add an ASSERT there instead of the NULL check. > > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:-1018 > > - webkit_web_resource_init_with_core_resource(webResource, coreResource.get()); > > Not entirely sure why this is removed. By mistake :). I think the final code should be something like this: const char* uri = webkit_web_resource_get_uri(webResource); RefPtr<ArchiveResource> coreResource(loader->subresource(KURL(KURL(), uri))); if (coreResource) webkit_web_resource_init_with_core_resource(webResource, coreResource.get()); I'd love to hear kov's opinion on the matter since he wrote most of that method.

(In reply to comment #10) > > > - // A NULL WebResource means the load has been interrupted, and > > > - // replaced by another one while this resource was being loaded. > > > - if (!webResource) > > > - return; > > > > So is the the case that webkit_web_view_get_resource can never return a NULL WebKitWebResource? If so you should probably ASSERT here. > > The implementation can return NULL, but since we register all the identifiers and WebCore will never notify us about a non existing resource, I think we can safely add an ASSERT there instead of the NULL check. By non-existing you mean 404? I'm pretty sure this comment made sense when it was written, I don't know if our assumptions have changed, though, IIRC this was for things like getting a resource load started which eventually got replaced by a plugin handling it, I'm not sure we won't be getting crashes from this =/ > By mistake :). I think the final code should be something like this: > > const char* uri = webkit_web_resource_get_uri(webResource); > RefPtr<ArchiveResource> coreResource(loader->subresource(KURL(KURL(), uri))); > if (coreResource) > webkit_web_resource_init_with_core_resource(webResource, coreResource.get()); > > I'd love to hear kov's opinion on the matter since he wrote most of that method. Are these changes required for the fix you're doing? I would prefer them to not be done at all or be landed separately at least, so we can more easily back them out.

Now that I thought a bit more about this I agree with Gustavo that we should not submit the patch even if it "fixes" the test case, basically because it would mean an API change, in the sense that some signals will mean a different thing. So once bug 93214 is submitted, the sequence of events in redirect-methods.html will be the right one although the test will not pass yet. There will be two text differences: (1) the uri of the WebResource (2) the lack of didFinishLoading message Those differences, as I mentioned, do not mean that the test is behaving incorrectly as they could be perfectly explained by the decisions taken when the WKGtk API was designed. I'll go through both cases: (1) The first argument in the "willSendRequest" message is supposed to be the URI of the resource that will change due to a redirect. In our case this fails because we rewrite the URI of the WebResouce (to the new one) before emitting the "resource-request-starting" signal in FrameLoaderClient::dispatchWillSendRequest. (2) WRT the lack of the didFinishLoading message in the test result, it can be explained if we carefully look at FrameLoaderClient::dispatchDidFinishLoading. If we do not find a coreResource for a subresource then we return and do not notify. From the comment above that code I guess this was added because we didn't want to emit "resource-load-finish" for resources that failed to load. What I have seen is that the lack of coreResource could also mean that the request was redirected (and thus a new resource was created to handle it). So putting aside the discussion about if this is the right way to do things or not from an API POV, I think that if we want to take advantage of passing such an important test (we have lately broke the redirection many times) we might consider the option of creating a specific test result for this case for our port (and even for WK1 only, I'd need to check the resources API in WK2) that will not print the new resource URI (as it is identical to the request) and won't have the didFinishLoading messages. wdyt?

Yeah, I think we should just target getting this passing for wk2 and leave the wk1 side as is if possible. No use spending much time in wk1 at this point.

Is this bug obsolete or is it the cause of https://bugzilla.gnome.org/show_bug.cgi?id=771856 ?

(In reply to comment #14) > Is this bug obsolete or is it the cause of > https://bugzilla.gnome.org/show_bug.cgi?id=771856 ? I guess the problem is still there indeed, but I haven't looked into it since I worked on this bug.

*** Bug 175189 has been marked as a duplicate of this bug. ***

Looks like this is breaking notifications on Google websites as well.

I don't think bug #175189 is actually a duplicate. It also looks like, reading this history of this bug, a fix has already been committed. It's not clear to me if there is still a bug here or not.

This can no longer be tested because Google+ closed down. Should we close this?

(In reply to Michael Catanzaro from comment #18) > It also looks like, reading this history of this bug, a fix has already been > committed. It's not clear to me if there is still a bug here or not. Sergio, if there's still a problem here, feel free to file an updated report.