Bug 34492 – avoid using invalidated KURL object in Element::baseURI

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Bryan Yeung]

Currently, an invalidated KURL object is used to carry around a relative URL string in Element::baseURI.  KURLGoogle does not support this behaviour, and I think it is better to not make use of invalidated KURL objects.

Here is a simple patch that makes no semantic change and circumvents the use of the invalidated object.

[Darin Adler]

(In reply to comment #0)
> Here is a simple patch that makes no semantic change and circumvents the use of
> the invalidated object.

Where?

[Darin Adler]

Please be clear, holding arbitrary strings that are not valid URLs is an intentional feature of KURL. To remove it we need to remove all clients that depend on that. It sure would be great to have the freedom to not need that feature, but we can’t get that by just fixing the cases we have already stumbled on, we need to look at all callers.

And then change the implementation so the mistake is easy to detect.

I know that Chromium has been living with all these bugs all along since KURLGoogle omitted that feature, but when fixing this please keep in mind that all other platforms have not been subject to the bugs.

KURLGoogle still seems like a bad idea to me. I know it’s pleasant for Chromium to share the same URL parsing with the other components in Chromium -- it would be nice to have that on the Mac too -- and someone may have the idea that it helps with security, but I believe that for the WebKit project as a whole would be great to make changes as needed to both the original KURL and KURLGoogle so their behavior is identical and then get rid of this unnecessary difference between platforms by deleting KURLGoogle.

[Bryan Yeung]

Created attachment 47952 [details]
patch

Does it not work to add a patch from the bug creation page?

Anyway, here is the patch.

[Bryan Yeung]

Sorry!  Wrong bug to attach the patch to.  Please ignore.

[Bryan Yeung]

(In reply to comment #4)
> Sorry!  Wrong bug to attach the patch to.  Please ignore.

Nope: I just didn't realize there were more comments, so this looked like a different bug.

Clearly I'm not really used to bugzilla.  Sorry for the confusion and extraneous comments.

[Darin Adler]

(In reply to comment #3)
> Does it not work to add a patch from the bug creation page?

I never tried it before. Maybe it’s broken.

[Darin Adler]

Comment on attachment 47952 [details]
patch

Change looks great, although I’d probably use assignment syntax rather than initialization to set up the baseAttribute local variable.

Generally it's not correct to set the review flag on a patch without a change log entry.

We require regression tests for bugs fixes. Since this patch fixes a bug, you must also include a regression test that fails on Chromium before this change and now succeeds with the fix. But neither the test itself nor the test results should be Chromium-specific.

review- because of the lack of change log and regression test

[Bryan Yeung]

Created attachment 47955 [details]
revised patch

Sorry for the incomplete patch last time.  This includes the ChangeLog entry.  There was already a test failing on Chromium without this change (LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg), so I do not think an additional regression test is necessary.

[Darin Adler]

(In reply to comment #8)
> Sorry for the incomplete patch last time.  This includes the ChangeLog entry. 
> There was already a test failing on Chromium without this change
> (LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg), so I do not think an
> additional regression test is necessary.

Typically a patch would include the corrected results or a change to the skipped list for a patch -- so I could see what test was changing behavior. But since Chromium has a different way of doing things, maybe that's why there's no such change here.

Despite the fact that a test was failing, a simpler test that targets this behavior more directly would be far better. It could even be a dumpAsText test.

Tests where success can only be judged from the pixel results are the lowest on our totem pole of test usefulness, so this existing test is only barely sufficient.

[Darin Adler]

Comment on attachment 47955 [details]
revised patch

> [phone]  Bryan Yeung  <bryeung@bryeung-chrome.(none)>

Strange address here.

r=me despite my concerns mentioned in comments above.

[WebKit Commit Bot]

Comment on attachment 47955 [details]
revised patch

Clearing flags on attachment: 47955

Committed r54245: <http://trac.webkit.org/changeset/54245>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

Committed r54264: <http://trac.webkit.org/changeset/54264>

[David Levin]

Rolled out due to major chromium bustage with this patch. 

Rolllout: http://trac.webkit.org/changeset/54264

[David Levin]

Actually, it looks like I may have been wrong. Once the canary bot gets well. I'd be happy to re-commit this (since it was rolled out incorrectly) -- sometimes it is hard to tell which patch caused the issue :(

[David Levin]

Committed again unchanged from before http://trac.webkit.org/changeset/54282

Sorry about the mix-up here.