Bug 77387 – Avoid Page::updateViewportArguments() if the causing frame is not the main frame

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Xianzhu Wang]

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.

[Xianzhu Wang]

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.

[WebKit Review Bot]

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.

[alan]

(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)

[Kenneth Rohde Christiansen]

(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

[Xianzhu Wang]

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.

[Kenneth Rohde Christiansen]

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)

[Xianzhu Wang]

(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.

[alan]

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())

[Xianzhu Wang]

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.

[alan]

> 
> >> 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.

[Kenneth Rohde Christiansen]

(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.

[Xianzhu Wang]

(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.

[Kenneth Rohde Christiansen]

(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).

[Kenneth Rohde Christiansen]

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

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 125741 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 125741 [details]
patch for landing

Clearing flags on attachment: 125741

Committed r106899: <http://trac.webkit.org/changeset/106899>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(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