[chromium] Fix unsafe viewport tag dispatch

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexandre Elias]

[chromium] Fix unsafe viewport tag dispatch

[Alexandre Elias]

Created attachment 130738 [details]
Patch

[Fady Samuel]

No new tests (already causes fixed-layout-360x240.html to fail when
ENABLE(VIEWPORT) is set).

That's because it treats the layout test as if it were a desktop page, and sets the fixed layout accordingly, avoiding the setting of the test. Do we want to avoid setting fixed layout manually in tests and instead test the viewport tag?

[Fady Samuel]

(In reply to comment #2)
> No new tests (already causes fixed-layout-360x240.html to fail when
> ENABLE(VIEWPORT) is set).
> 
> That's because it treats the layout test as if it were a desktop page, and sets the fixed layout accordingly, avoiding the setting of the test. Do we want to avoid setting fixed layout manually in tests and instead test the viewport tag?

Why would we not always have access to the screen DPI? This seems like a property independent of layout...

[Alexandre Elias]

(In reply to comment #3)
> (In reply to comment #2)
> > No new tests (already causes fixed-layout-360x240.html to fail when
> > ENABLE(VIEWPORT) is set).
> > 
> > That's because it treats the layout test as if it were a desktop page, and sets the fixed layout accordingly, avoiding the setting of the test. Do we want to avoid setting fixed layout manually in tests and instead test the viewport tag?

Hmm, on a second look, it's more of a coincidence that this test was failing.  It was actually failing on the ASSERT(dpi > 0) because the test harness didn't provide a value for it.  I think we can leave that test as is (as long as we get rid of the assertion), but I'll see if there's a way to reliably test the actual bug I saw.

> 
> Why would we not always have access to the screen DPI? This seems like a property independent of layout...

True, but we currently ask the browser process on Android, mainly to avoid code duplication.  Because we also need to go to the browser process for the window size anyway, it doesn't seem like there's any benefit to guaranteeing the dpi value is always available.

[Fady Samuel]

> Hmm, on a second look, it's more of a coincidence that this test was failing.  It was actually failing on the ASSERT(dpi > 0) because the test harness didn't provide a value for it.  I think we can leave that test as is (as long as we get rid of the assertion), but I'll see if there's a way to reliably test the actual bug I saw.
> 

Hmm, I recall seeing it fail when viewport was enabled as well...

> > Why would we not always have access to the screen DPI? This seems like a property independent of layout...
> 
> True, but we currently ask the browser process on Android, mainly to avoid code duplication.  Because we also need to go to the browser process for the window size anyway, it doesn't seem like there's any benefit to guaranteeing the dpi value is always available.

Do we need to go to the browser for window size? Can we not use the WebView's size, instead?

[Alexandre Elias]

(In reply to comment #5)
> > > Why would we not always have access to the screen DPI? This seems like a property independent of layout...
> > 
> > True, but we currently ask the browser process on Android, mainly to avoid code duplication.  Because we also need to go to the browser process for the window size anyway, it doesn't seem like there's any benefit to guaranteeing the dpi value is always available.
> 
> Do we need to go to the browser for window size? Can we not use the WebView's size, instead?

My (second-hand) understanding is that the browser is the decider of window sizes.  For example, in a popup ad in Windows, WebKit will start loading the content from the network at the same time as a request is issued to Windows to create/position a window such that it fits on the screen.  So the window size may not be available to the WebView early in loading.  Of course, on mobile the situation is typically simpler than that, but we have the same architecture.

[Fady Samuel]

(In reply to comment #6)
> (In reply to comment #5)
> > > > Why would we not always have access to the screen DPI? This seems like a property independent of layout...
> > > 
> > > True, but we currently ask the browser process on Android, mainly to avoid code duplication.  Because we also need to go to the browser process for the window size anyway, it doesn't seem like there's any benefit to guaranteeing the dpi value is always available.
> > 
> > Do we need to go to the browser for window size? Can we not use the WebView's size, instead?
> 
> My (second-hand) understanding is that the browser is the decider of window sizes.  For example, in a popup ad in Windows, WebKit will start loading the content from the network at the same time as a request is issued to Windows to create/position a window such that it fits on the screen.  So the window size may not be available to the WebView early in loading.  Of course, on mobile the situation is typically simpler than that, but we have the same architecture.

One of my concerns is IPCs are fairly expensive operations and this method is called fairly frequently. I was hoping to send out a patch to get rid of the use of IPCs here, but I didn't study the implications of this change enough.

[Alexandre Elias]

(In reply to comment #7)
> One of my concerns is IPCs are fairly expensive operations and this method is called fairly frequently. I was hoping to send out a patch to get rid of the use of IPCs here, but I didn't study the implications of this change enough.

Is it really that frequent?  It seems like after this CL, it should be called at most twice on initialization.

Anyway, I think it should be possible to invert the IPC relationship in the embedder so that the browser sends the size to the renderer, instead of having a sync IPC the other way.  But for the purposes of the code in WebKit I'm changing here, I don't think that changes the fact that the GetWindowRect function may sometimes return no value.

[Alexandre Elias]

Created attachment 131483 [details]
Patch

[Alexandre Elias]

I added a unit test and also a small fix to the navigation reset logic (found after trying out the change some more).  PTAL.

[Alexandre Elias]

Created attachment 135181 [details]
Patch

[Alexandre Elias]

Uploaded a new patch with a few more tweaks.  I left in the dpi assertion after all, as we are now guaranteed to get it on render process initialization on Android, so that resolves that debate.

[Alexandre Elias]

Created attachment 135190 [details]
Patch

[Alexandre Elias]

After discussing with Fady, removed the initial-scale related fixes from this patch, I'll make a separate bug for them.

[Alexandre Elias]

Created attachment 135233 [details]
Patch

[Alexandre Elias]

I moved the test into the initial-scale bug http://webk.it/82949 where it's a better fit.

[Alexandre Elias]

Created attachment 138486 [details]
Patch

[Alexandre Elias]

I added another call site to WebViewImpl::resize() as that's another place where the inputs to the viewport calculation can change.  I also switched the guards to use the new viewportEnabled setting.

[WebKit Review Bot]

Comment on attachment 138486 [details]
Patch

Attachment 138486 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12519271

New failing tests:
WebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag

[WebKit Review Bot]

Created attachment 138575 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexandre Elias]

Created attachment 138615 [details]
Patch

[Adam Barth]

Fady, are you happy with this patch now?

[Fady Samuel]

(In reply to comment #22)
> Fady, are you happy with this patch now?

LGTM! Thanks! :-)

[Adam Barth]

Comment on attachment 138615 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1304
> +        Document* document = mainFrameImpl()->frame()->document();
> +        if (document) {

The frame should always have a document.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1306
> +          ViewportArguments viewportArguments = document->viewportArguments();
> +          m_page->chrome()->client()->dispatchViewportPropertiesDidChange(viewportArguments);

These lines aren't indented correctly.  WebKit uses four-space indent.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2966
> +        if (!document)
> +            return;

This should be impossible.

[Alexandre Elias]

Created attachment 141060 [details]
Patch

[Alexandre Elias]

All done.

[Adam Barth]

Comment on attachment 141060 [details]
Patch

Sorry for the delay.

[WebKit Review Bot]

Comment on attachment 141060 [details]
Patch

Clearing flags on attachment: 141060

Committed r117170: <http://trac.webkit.org/changeset/117170>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

I am a bit afraid that removing this from FrameView will break Qt. I am cc'ing Zalan to see if this is the case.

Zalan it might make sense to write a autotest similar to the one apparently asserting for Chromium. That should be possible using my new UI side testing framework.

[Alexandre Elias]

(In reply to comment #30)
> I am a bit afraid that removing this from FrameView will break Qt. I am cc'ing Zalan to see if this is the case.

It was initially added for Chromium in bug 80554 (December), hopefully no other ports have come to rely on it.

[Alexandre Elias]

(In reply to comment #31)
> (In reply to comment #30)
> > I am a bit afraid that removing this from FrameView will break Qt. I am cc'ing Zalan to see if this is the case.
> 
> It was initially added for Chromium in bug 80554 (December), hopefully no other ports have come to rely on it.

Sorry, I meant bug 73495 :)