Bug 80554 - [chromium] Fix unsafe viewport tag dispatch
Summary: [chromium] Fix unsafe viewport tag dispatch
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandre Elias
URL:
Keywords:
Depends on:
Blocks: 66687 70559 82949
  Show dependency treegraph
 
Reported: 2012-03-07 17:35 PST by Alexandre Elias
Modified: 2012-05-15 16:14 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.49 KB, patch)
2012-03-07 17:49 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (9.71 KB, patch)
2012-03-12 18:22 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (9.65 KB, patch)
2012-04-02 13:59 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (8.23 KB, patch)
2012-04-02 14:33 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (6.33 KB, patch)
2012-04-02 17:11 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (7.14 KB, patch)
2012-04-23 19:07 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.13 MB, application/zip)
2012-04-24 09:06 PDT, WebKit Review Bot
no flags Details
Patch (8.02 KB, patch)
2012-04-24 12:06 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2012-05-09 17:13 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2012-03-07 17:35:42 PST
[chromium] Fix unsafe viewport tag dispatch
Comment 1 Alexandre Elias 2012-03-07 17:49:35 PST
Created attachment 130738 [details]
Patch
Comment 2 Fady Samuel 2012-03-08 11:04:57 PST
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?
Comment 3 Fady Samuel 2012-03-08 11:24:42 PST
(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...
Comment 4 Alexandre Elias 2012-03-08 12:07:15 PST
(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.
Comment 5 Fady Samuel 2012-03-08 12:15:28 PST
> 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?
Comment 6 Alexandre Elias 2012-03-08 12:25:02 PST
(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.
Comment 7 Fady Samuel 2012-03-08 12:31:41 PST
(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.
Comment 8 Alexandre Elias 2012-03-08 12:35:37 PST
(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.
Comment 9 Alexandre Elias 2012-03-12 18:22:08 PDT
Created attachment 131483 [details]
Patch
Comment 10 Alexandre Elias 2012-03-12 18:25:20 PDT
I added a unit test and also a small fix to the navigation reset logic (found after trying out the change some more).  PTAL.
Comment 11 Alexandre Elias 2012-04-02 13:59:41 PDT
Created attachment 135181 [details]
Patch
Comment 12 Alexandre Elias 2012-04-02 14:03:16 PDT
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.
Comment 13 Alexandre Elias 2012-04-02 14:33:10 PDT
Created attachment 135190 [details]
Patch
Comment 14 Alexandre Elias 2012-04-02 14:34:31 PDT
After discussing with Fady, removed the initial-scale related fixes from this patch, I'll make a separate bug for them.
Comment 15 Alexandre Elias 2012-04-02 17:11:41 PDT
Created attachment 135233 [details]
Patch
Comment 16 Alexandre Elias 2012-04-02 17:15:48 PDT
I moved the test into the initial-scale bug http://webk.it/82949 where it's a better fit.
Comment 17 Alexandre Elias 2012-04-23 19:07:03 PDT
Created attachment 138486 [details]
Patch
Comment 18 Alexandre Elias 2012-04-23 19:08:49 PDT
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.
Comment 19 WebKit Review Bot 2012-04-24 09:05:49 PDT
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
Comment 20 WebKit Review Bot 2012-04-24 09:06:05 PDT
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
Comment 21 Alexandre Elias 2012-04-24 12:06:12 PDT
Created attachment 138615 [details]
Patch
Comment 22 Adam Barth 2012-05-09 16:29:34 PDT
Fady, are you happy with this patch now?
Comment 23 Fady Samuel 2012-05-09 16:31:45 PDT
(In reply to comment #22)
> Fady, are you happy with this patch now?

LGTM! Thanks! :-)
Comment 24 Adam Barth 2012-05-09 16:59:04 PDT
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.
Comment 25 Alexandre Elias 2012-05-09 17:13:41 PDT
Created attachment 141060 [details]
Patch
Comment 26 Alexandre Elias 2012-05-09 17:13:58 PDT
All done.
Comment 27 Adam Barth 2012-05-15 15:17:35 PDT
Comment on attachment 141060 [details]
Patch

Sorry for the delay.
Comment 28 WebKit Review Bot 2012-05-15 15:33:26 PDT
Comment on attachment 141060 [details]
Patch

Clearing flags on attachment: 141060

Committed r117170: <http://trac.webkit.org/changeset/117170>
Comment 29 WebKit Review Bot 2012-05-15 15:33:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Kenneth Rohde Christiansen 2012-05-15 15:46:57 PDT
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.
Comment 31 Alexandre Elias 2012-05-15 16:13:01 PDT
(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.
Comment 32 Alexandre Elias 2012-05-15 16:14:01 PDT
(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 :)