Closed Bug 1279202 Opened 8 years ago Closed 8 years ago

Google Docs confirmation dialog when sharing doesn't show buttons

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
platform-rel --- +
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 + fixed
firefox-esr45 50+ fixed
firefox50 + fixed

People

(Reporter: jdm, Assigned: mattwoodrow)

References

Details

(Keywords: regression, Whiteboard: [platform-rel-Google][platform-rel-GoogleDocs])

Attachments

(1 file, 1 obsolete file)

Make sure we flush layout when we have a deferred resize v2
(deleted), patch
mattwoodrow
: review+
Sylvestre
: approval-mozilla-aurora+
lizzard
: approval-mozilla-esr45+
Details | Diff | Splinter Review
STR: 1. make a new google document 2. share it with an email address that is not associated with a google account Expected results: A dialog pops up saying "Are you sure?" describing how sharing with that account could be unsafe, and confirmation and cancellation buttons appear. Actual results: A dialog pops up saying "Are you sure?" but contains no other content, and there's no way to dismiss it. I see this on a nightly release with e10s enabled, and I don't see this in FF 46.0.1 with e10s disabled. I'm going to try to narrow the window and variables involved in this regression.
Nightly without e10s shows the issue and FF 48 does not, so I'm bisecting.
Requesting tracking for making a google doc unusable when trying to share with any non-google email accounts.
status-firefox47: --- → unaffected
status-firefox48: --- → unaffected
status-firefox49: --- → affected
status-firefox50: --- → affected
tracking-firefox49: --- → ?
tracking-firefox50: --- → ?
I'm bisecting inbound, but I'm placing my bets on bug 881832.
This regression was caused by https://hg.mozilla.org/mozilla-central/rev/26e22ea9e8dd.
Blocks: 881832
Flags: needinfo?(matt.woodrow)
Component: General → Layout
Whiteboard: [platform-rel-Google][platform-rel-GoogleDocs]
Tracking 50+ since this regression is user visible.
tracking-firefox50: ? → +
platform-rel: --- → ?
Tracking 49+ as well.
tracking-firefox49: ? → +
Component: Layout → Layout: HTML Frames
Maybe Mats can look at this?
Flags: needinfo?(mats)
Sorry, not before September or so.
Flags: needinfo?(mats)
Jet, this needs an assignee, and probably needs to be fixed for 49
Flags: needinfo?(bugs)
Judging by the STR, I'm going to want to use RR to debug this in a reasonable timeframe, and I don't have that machine with me right I'll. I'll take this and work on it the week after next (the 11th) unless someone unless wants to take it before then. A reduced testcase would be ideal :)
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugs)
Version: unspecified → 49 Branch
Google docs is querying 'clientWidth' and 'clientHeight' on the <html> element within the iframe, and we're returning the old size where previously we had synchronously resized and returned the new size. It then uses these (wrong) sizes and positions the text content incorrectly. It's still there, just has a large 'top' value that stops it from being visible. It doesn't look like querying clientWidth/Height forces us to flush, should it?
Flags: needinfo?(dbaron)
It does seem like it should flush what is needed. Right now it does look like it does a flush, via the following path: Element::ClientWidth -> Element::GetClientAreaRect -> Element::GetScrollFrame (aFlushLayout=true) -> Element::GetPrimaryFrame(aType=Flush_Layout) -> nsDocument::FlushPendingNotifications(aType=Flush_Layout) nsDocument::FlushPendingNotifications *should* call nsPresShell::FlushPendingNotifications -- though it's worth double-checking that mNeedLayoutFlush is set correctly. It seems likely that the underlying bug here is failure to call nsIDocument::SetNeedLayoutFlush. If it's not, another area that might be worth examining is the code that propagates a flush from a child document into flushing layout in the parent document, which we need to do in some cases (layout flushes in the child, style flushes in the child when media queries are involved).
Flags: needinfo?(dbaron)
Thanks David, this turned out to be quite obvious in the end. The testcase was a pain, needing the double style flush was non-obvious.
Attachment #8770408 - Flags: review?(dbaron)
So if I'm reading nsDocument::FlushPendingNotifications, this will mean that if we have a pending resize, then nsDocument::FlushPendingNotifications(Flush_Style) will no longer call through to the pres shell, since mNeedStyleFlush will no longer be true. I need to look into why that line exists...
So there are two separate mViewManager->FlushDelayedResize() calls in PresShell::FlushPendingNotifications. The first passes aDoReflow = false, and is conditional only on it being safe to call. This is needed to get styles correct because of media queries. The second passes aDoReflow = true, and is also conditional on flushType, so it does not happen for Flush_Style. Given comment 15, I think this means that the patch should be adding a SetNeedLayoutFlush, but not removing the SetNeedStyleFlush call, since removing the SetNeedStyleFlush call could cause a Flush_Style to fail to flush the pres shell. (A Flush_Style happens for things like getComputedStyle().getPropertyValue("color"), where "color" must be a property that doesn't have CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH.)
Comment on attachment 8770408 [details] [diff] [review] Make sure we flush layout when we have a deferred resize If you agree with the reasoning in comment 15 and comment 16, then r=dbaron with the removal of the SetNeedStyleFlush removed. (You probably want to put mPresShell->GetDocument() in a local variable given that you'll use it twice.) It would probably also be good to add a testcase that fails *with* this patch because of the lack of a SetNeedStyleFlush.
Attachment #8770408 - Flags: review?(dbaron) → review+
As far as I can tell, the code here has been broken since bug 709256 (Gecko 11). It's just that delayed resizes in foreground tabs weren't Web-exposed until bug 881832 (Gecko 49). But I suspect the underlying bug probably has been causing breakage in background tabs.
Blocks: 709256
(But the only thing I found in bugzilla that appears to have even a remote possibility of being related is bug 1005964.)
Fixed review comments and added a second test condition to make sure both style and reflow are flushed correctly. Carrying r=dbaron.
Attachment #8770408 - Attachment is obsolete: true
Attachment #8770755 - Flags: review+
Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6e45ae207f Make sure that deferring a resize of a document schedules a layout flush, not just a style one. r=dbaron
Comment on attachment 8770755 [details] [diff] [review] Make sure we flush layout when we have a deferred resize v2 Just to double-check -- did you test that this test: >+ is(color, "rgb(0, 128, 0)", "Style flush not completed when resizing an iframe!"); fails with the patch you initially posted for review, and this test: >+ is(width, 300, "Layout flush not completed when resizing an iframe!"); fails with the code before your patch?
Flags: needinfo?(matt.woodrow)
platform-rel: ? → +
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #22) > Comment on attachment 8770755 [details] [diff] [review] > Make sure we flush layout when we have a deferred resize v2 > > Just to double-check -- did you test that this test: > >+ is(color, "rgb(0, 128, 0)", "Style flush not completed when resizing an iframe!"); > fails with the patch you initially posted for review, and this test: > >+ is(width, 300, "Layout flush not completed when resizing an iframe!"); > fails with the code before your patch? Yes I did! :)
Flags: needinfo?(matt.woodrow)
https://hg.mozilla.org/mozilla-central/rev/8f6e45ae207f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Nice. Please request uplift to 49.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8770755 [details] [diff] [review] Make sure we flush layout when we have a deferred resize v2 Approval Request Comment [Feature/regressing bug #]: Bug 881832. [User impact if declined]: Incorrect layout dimensions exposed to JS in rare occasions, affects google docs. [Describe test coverage new/current, TreeHerder]: Manually tested, new mochitests added. [Risks and why]: Very low risk, just ensures that a layout flush is done before we return sizes to JS. [String/UUID change made/needed]: None.
Flags: needinfo?(matt.woodrow)
Attachment #8770755 - Flags: approval-mozilla-aurora?
Comment on attachment 8770755 [details] [diff] [review] Make sure we flush layout when we have a deferred resize v2 Fix a new regression, taking it.
Attachment #8770755 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8770755 [details] [diff] [review] Make sure we flush layout when we have a deferred resize v2 We need this to land along with some other work from bug 928187. This should land for 45.5.0esr. The release date for ESR has changed, and is now Nov. 15.
Comment on attachment 8770755 [details] [diff] [review] Make sure we flush layout when we have a deferred resize v2 Taking this as part of the work uplifting to ESR from bug 881832.
Attachment #8770755 - Flags: approval-mozilla-esr45+
has problems to apply to esr grafting 357106:5b2eda73f0cc "Bug 1279202 - Make sure that deferring a resize of a document schedules a layout flush, not just a style one. r=dbaron, a=sledru" merging layout/base/tests/mochitest.ini merging view/nsViewManager.cpp warning: conflicts while merging layout/base/tests/mochitest.ini! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(matt.woodrow)
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: