Bug 77387 - Avoid Page::updateViewportArguments() if the causing frame is not the main frame
Summary: Avoid Page::updateViewportArguments() if the causing frame is not the main frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-30 16:52 PST by Xianzhu Wang
Modified: 2012-02-07 01:11 PST (History)
9 users (show)

See Also:


Attachments
patch (for preview, no test) (2.96 KB, patch)
2012-01-31 12:17 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch v2 (moved updateViewportArguments from Page to Document) (5.83 KB, patch)
2012-02-03 10:53 PST, Xianzhu Wang
kenneth: review+
Details | Formatted Diff | Diff
patch for landing (6.15 KB, patch)
2012-02-06 17:46 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2012-01-30 16:52:31 PST
As Page::updateViewportArguments() only processes the ViewPortArguments of the main frame document, it's wasteful to call it from frame/document which is not the main frame/document.

I saw a crash in my local testing environment caused a call to the method from a subframe.
The call stack is like the following:

  FrameView::layout() (subframe) crashed because document is null
  ...
  FrameView::layout() (main frame)
  ...
  ScrollView::updateScrollbars()
  ...
  Page::updateViewportArguments()
  Frame::setDocument(0) (subframe)
  FrameLoader::clear()
  DocumentWriter::begin()
  ...

Will create a test case when have time.
Comment 1 Xianzhu Wang 2012-01-31 12:17:17 PST
Created attachment 124794 [details]
patch (for preview, no test)

This patch doesn't contain a new layout test. Uploaded for preview to see if the method is correct.
I've run all the layout tests on chromium-linux and there was no difference before and after the change.
Comment 2 WebKit Review Bot 2012-01-31 12:19:35 PST
Attachment 124794 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/WebCore.exp.in
Auto-merging Source/WebKit/mac/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 zalan 2012-02-01 09:37:42 PST
(In reply to comment #0)
> As Page::updateViewportArguments() only processes the ViewPortArguments of the main frame document, it's wasteful to call it from frame/document which is not the main frame/document.
It is, indeed. However checking against mainframe() before calling updateViewportArguments() does not prevent anybody from adding a new call without the mainframe check, which could end up in a segfault again.

I'd move updateViewportArguments() from Page to Document and do something like this:

Document::updateViewportArguments()
{
  if (frame() != frame()->page()->mainFrame()) // missing few checks.
      return;
  ...
  page()->chrome()->dispatchViewportPropertiesDidChange();
  ...
}

Obviously anyone can still make mainframe()->document()->updateViewportArguments() call, but that might look more suspicious and raise questions while reviewing, than a new frame->page()->updateViewportArguments() call without the mainframe() check.
Though, it is rather subjective.

(Also, Page::m_viewportArguments looks redundant to me)
Comment 4 Kenneth Rohde Christiansen 2012-02-02 02:29:20 PST
(In reply to comment #3)
> (In reply to comment #0)
> > As Page::updateViewportArguments() only processes the ViewPortArguments of the main frame document, it's wasteful to call it from frame/document which is not the main frame/document.
> It is, indeed. However checking against mainframe() before calling updateViewportArguments() does not prevent anybody from adding a new call without the mainframe check, which could end up in a segfault again.
> 
> I'd move updateViewportArguments() from Page to Document and do something like this:
> 
> Document::updateViewportArguments()
> {
>   if (frame() != frame()->page()->mainFrame()) // missing few checks.
>       return;
>   ...
>   page()->chrome()->dispatchViewportPropertiesDidChange();
>   ...
> }
> 
> Obviously anyone can still make mainframe()->document()->updateViewportArguments() call, but that might look more suspicious and raise questions while reviewing, than a new frame->page()->updateViewportArguments() call without the mainframe() check.
> Though, it is rather subjective.
> 
> (Also, Page::m_viewportArguments looks redundant to me)

You could assert as well, making sure people only call it from the right place
Comment 5 Xianzhu Wang 2012-02-03 10:53:25 PST
Created attachment 125366 [details]
patch v2 (moved updateViewportArguments from Page to Document)

Thanks for suggestions. Updated patch. Not using ASSERT because I don't want the callers to know how viewport update and the main frame are related and to check that.
Comment 6 Kenneth Rohde Christiansen 2012-02-06 03:58:17 PST
Comment on attachment 125366 [details]
patch v2 (moved updateViewportArguments from Page to Document)

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

> Source/WebCore/ChangeLog:5
> +        Avoid Page::updateViewportArguments() if the causing frame is not the main frame
> +        https://bugs.webkit.org/show_bug.cgi?id=77387
> +

I am not sure how this should affect frames going to fullscreen (http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html)
Comment 7 Xianzhu Wang 2012-02-06 09:17:04 PST
(In reply to comment #6)
> I am not sure how this should affect frames going to fullscreen (http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html)

Is fullscreen API covered by the current layout tests (I saw some fullscreen tests)? If yes, I have verified that my patch won't affect any existing layout tests.
Comment 8 zalan 2012-02-06 11:31:47 PST
Comment on attachment 125366 [details]
patch v2 (moved updateViewportArguments from Page to Document)

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

> Source/WebCore/dom/Document.cpp:2807
> +        page()->chrome()->dispatchViewportPropertiesDidChange(m_viewportArguments);

Not super important, but i'd check if the frame() is NULL. If both mainframe and frame are NULL, the viewport prop. change is dispatched. Though I'm not sure if it can ever happen.

> Source/WebCore/html/HTMLBodyElement.cpp:186
> +    document()->updateViewportArguments();

missing if (document())
Comment 9 Xianzhu Wang 2012-02-06 11:43:19 PST
Comment on attachment 125366 [details]
patch v2 (moved updateViewportArguments from Page to Document)

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

>> Source/WebCore/dom/Document.cpp:2807
>> +        page()->chrome()->dispatchViewportPropertiesDidChange(m_viewportArguments);
> 
> Not super important, but i'd check if the frame() is NULL. If both mainframe and frame are NULL, the viewport prop. change is dispatched. Though I'm not sure if it can ever happen.

As page() is '{ m_frame ? m_frame->page() : 0; }, frame() won't be NULL if page() is not NULL.

>> Source/WebCore/html/HTMLBodyElement.cpp:186
>> +    document()->updateViewportArguments();
> 
> missing if (document())

document() won't be NULL, because here is insertedIntoDocument(). Also there are at least 2 "document()->" in the function.
Comment 10 zalan 2012-02-06 11:47:42 PST
> 
> >> Source/WebCore/html/HTMLBodyElement.cpp:186
> >> +    document()->updateViewportArguments();
> > 
> > missing if (document())
> 
> document() won't be NULL, because here is insertedIntoDocument(). Also there are at least 2 "document()->" in the function.
good, i was just looking at the diff, where the old code had the document() check.
Comment 11 Kenneth Rohde Christiansen 2012-02-06 11:55:26 PST
(In reply to comment #7)
> (In reply to comment #6)
> > I am not sure how this should affect frames going to fullscreen (http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html)
> 
> Is fullscreen API covered by the current layout tests (I saw some fullscreen tests)? If yes, I have verified that my patch won't affect any existing layout tests.

I don't believe there are any tests where for instance and iframe becomes fullscreen and that has a viewport meta tag, but it should be a valid use-case.
Comment 12 Xianzhu Wang 2012-02-06 13:56:04 PST
(In reply to comment #11)
> I don't believe there are any tests where for instance and iframe becomes fullscreen and that has a viewport meta tag, but it should be a valid use-case.

Without the patch, updateViewportArguments() is called more frequently when the viewports of sub-frames change, but only the viewport of the main frame/document is used. I wonder if a sub-frame will become the main frame when it goes full-screen. If not, this should be another bug independent with this one.
Comment 13 Kenneth Rohde Christiansen 2012-02-06 15:23:37 PST
(In reply to comment #12)
> (In reply to comment #11)
> > I don't believe there are any tests where for instance and iframe becomes fullscreen and that has a viewport meta tag, but it should be a valid use-case.
> 
> Without the patch, updateViewportArguments() is called more frequently when the viewports of sub-frames change, but only the viewport of the main frame/document is used. I wonder if a sub-frame will become the main frame when it goes full-screen. If not, this should be another bug independent with this one.

I do not believe that they become the main frame, but I might be wrong. I also do not think that anyone considered this use case when combining the Fullscreen spec with CSS Device Adaption.

The question is does the CSS Device Adaption spec apply to fullscreen iframe and even fullscreen elements as it now supports setting a viewport using CSS (which we do not support yet).

Could you look into this or open a bug (cc me please).
Comment 14 Kenneth Rohde Christiansen 2012-02-06 15:24:34 PST
Comment on attachment 125366 [details]
patch v2 (moved updateViewportArguments from Page to Document)

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

>>> Source/WebCore/html/HTMLBodyElement.cpp:186
>>> +    document()->updateViewportArguments();
>> 
>> missing if (document())
> 
> document() won't be NULL, because here is insertedIntoDocument(). Also there are at least 2 "document()->" in the function.

You could add an ASSERT just in case
Comment 15 Xianzhu Wang 2012-02-06 17:44:03 PST
Comment on attachment 125366 [details]
patch v2 (moved updateViewportArguments from Page to Document)

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

>>>> Source/WebCore/html/HTMLBodyElement.cpp:186
>>>> +    document()->updateViewportArguments();
>>> 
>>> missing if (document())
>> 
>> document() won't be NULL, because here is insertedIntoDocument(). Also there are at least 2 "document()->" in the function.
> 
> You could add an ASSERT just in case

Done.
Comment 16 Xianzhu Wang 2012-02-06 17:46:26 PST
Created attachment 125741 [details]
patch for landing
Comment 17 WebKit Review Bot 2012-02-06 20:17:10 PST
Comment on attachment 125741 [details]
patch for landing

Clearing flags on attachment: 125741

Committed r106899: <http://trac.webkit.org/changeset/106899>
Comment 18 WebKit Review Bot 2012-02-06 20:17:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Philippe Normand 2012-02-07 01:11:59 PST
(In reply to comment #17)
> (From update of attachment 125741 [details])
> Clearing flags on attachment: 125741
> 
> Committed r106899: <http://trac.webkit.org/changeset/106899>

FTR this broke on GTK :( See bug 77943