Last Comment Bug 764467 - resize event handler in browser.js gets triggered at unexpected times
: resize event handler in browser.js gets triggered at unexpected times
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 16
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on:
Blocks: 758633 766556 779811
  Show dependency treegraph
 
Reported: 2012-06-13 10:19 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-09-06 06:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Make a new event (3.36 KB, patch)
2012-06-13 11:21 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2012-06-13 10:19:00 PDT
Right now when we send resize events from java to gecko, they don't get dispatched to the resize handler in browser.js right away. Instead, it sets a flag somewhere in Gecko, and then next time a layout property is requested it will trigger the resize handling. Oftentimes the layout property is requested from other code in browser.js, like in scrollToFocusedInput or in getViewport(). This is undesirable, particularly in getViewport() because the resize handler changes gScreenWidth and gScreenHeight, which might make getViewport() return an inconsistent viewport.

Ideally the "resize" event should be handled as soon as possible. One solution to this problem could be to change browser.js to not listen to the gecko-sent "resize" event, but instead send a new broadcast message straight from java to browser.js with the new screen size. Another would be to force a flush of the "resize" event pending in gecko from nsWindow somehow.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-13 11:21:26 PDT
Created attachment 632775 [details] [diff] [review]
Make a new event
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-13 11:25:46 PDT
In case comment 0 isn't clear, the existing resize handler causes re-entrancy into various chunks of code in browser.js. For example:

1. getViewport() is called
2. getViewport() requests window.scrollX
3. this triggers a flush of pending layout
4. this triggers dispatching of pending resize events
5. the resize handler in browser.js gets run
6. this calls getViewport() and generally mucks about with the viewport
7. finally everything unwinds to the original call to getViewport() in 1. that call already used the old value of gScreenWidth and gScreenHeight and so now returns something inconsistent.

Similar problems could happen just about anywhere a layout property is requested. In fact, the existing resize handler was triggering more resize events because of the changes to the CSS viewport, which made things even more messy. The new handler is triggered only once per actual resize, and gecko does whatever layout flushing it needs to.
Comment 3 Chris Lord [:cwiiis] 2012-06-13 22:58:27 PDT
Comment on attachment 632775 [details] [diff] [review]
Make a new event

Review of attachment 632775 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me. I assume that this isn't strictly re-entrancy (in that, it enters the same function recursively)? Because if it is, I think it'd be nicer to guard against that with a counter than to introduce a new message.
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-14 06:56:00 PDT
(In reply to Chris Lord [:cwiiis] from comment #3)
> Looks reasonable to me. I assume that this isn't strictly re-entrancy (in
> that, it enters the same function recursively)? Because if it is, I think
> it'd be nicer to guard against that with a counter than to introduce a new
> message.

Without this patch, it does enter the same function recursively. The sequence listed on comment #2, for example, starts off in getViewport() in step 1, and while it's in there, re-enters recursively in step 6.

With the patch, however, there is no re-entrancy/recursion.
Comment 5 Chris Lord [:cwiiis] 2012-06-14 06:59:12 PDT
(In reply to Kartikaya Gupta (:kats) from comment #4)
> (In reply to Chris Lord [:cwiiis] from comment #3)
> > Looks reasonable to me. I assume that this isn't strictly re-entrancy (in
> > that, it enters the same function recursively)? Because if it is, I think
> > it'd be nicer to guard against that with a counter than to introduce a new
> > message.
> 
> Without this patch, it does enter the same function recursively. The
> sequence listed on comment #2, for example, starts off in getViewport() in
> step 1, and while it's in there, re-enters recursively in step 6.
> 
> With the patch, however, there is no re-entrancy/recursion.

ok, would it be possible to just set a global boolean at the start of the function and return if it's true? I'm just thinking of the overhead of the extra message - I don't suppose it's much, but it's another thing in a long list of things to keep track of. I don't have particularly strong feelings about it though, so down to you.
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-14 07:28:47 PDT
(In reply to Chris Lord [:cwiiis] from comment #5)
> ok, would it be possible to just set a global boolean at the start of the
> function and return if it's true?

Well, it would have to return some value. I tried this at some point before, and the only "correct" code I could come up with wound up looking something like this:

exitedGetViewport: true,

getViewport: function() {
  exitedGetViewport = false;
  ... calculate "viewport" ...
  if (exitedGetViewport) {
    // this indicated that we re-entered getViewport during calculation above
    // so the viewport we calculated is now borked. re-calculate it
    return getViewport();
  }
  exitedGetViewport = true;
  return viewport;
}

And then there's the possibility that other functions have this problem too (not currently, but might happen in the future).
Comment 7 Chris Lord [:cwiiis] 2012-06-14 07:33:00 PDT
(In reply to Kartikaya Gupta (:kats) from comment #6)
> (In reply to Chris Lord [:cwiiis] from comment #5)
> > ok, would it be possible to just set a global boolean at the start of the
> > function and return if it's true?
> 
> Well, it would have to return some value. I tried this at some point before,
> and the only "correct" code I could come up with wound up looking something
> like this:
> 
> exitedGetViewport: true,
> 
> getViewport: function() {
>   exitedGetViewport = false;
>   ... calculate "viewport" ...
>   if (exitedGetViewport) {
>     // this indicated that we re-entered getViewport during calculation above
>     // so the viewport we calculated is now borked. re-calculate it
>     return getViewport();
>   }
>   exitedGetViewport = true;
>   return viewport;
> }
> 
> And then there's the possibility that other functions have this problem too
> (not currently, but might happen in the future).

ok, that's much uglier, fair enough :)
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-14 08:00:54 PDT
Yeah, it really is :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1df07bc54e5
Comment 9 Ed Morley [:emorley] 2012-06-15 06:00:02 PDT
https://hg.mozilla.org/mozilla-central/rev/c1df07bc54e5
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-02 12:03:29 PDT
Comment on attachment 632775 [details] [diff] [review]
Make a new event

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: bug 766556 occurs on mozilla-beta without this patch, because the patch for that depends on the Window:Resize event added here
Testing completed (on m-c, etc.): been baking on m-c/aurora for a while now
Risk to taking this patch (and alternatives if risky): mobile only, very low risk given that it's been baking so long with no ill-effects
String or UUID changes made by this patch: none
Comment 11 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-02 12:09:32 PDT
Note: bug 779811 was filed as well for essentially the same problem as bug 766556. Personally I'm seeing bug 766556 on my 15.0b2 install on the galaxy nexus, I don't need to zoom to see it happen like bug 779811 specifies. I'm linking both bugs here just to cover all the bases.
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-03 11:27:54 PDT
Comment on attachment 632775 [details] [diff] [review]
Make a new event

low risk, mobile only, fixes issues with text selection handling - approving for beta.
Comment 13 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-07 07:12:32 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/b12557028a78
Comment 14 Cristian Nicolae (:xti) 2012-09-06 06:53:28 PDT
The last crash occurred almost a week ago on 17.0a1 build. I cannot reproduce it on any branch. Closing bug as verified fixed.

--
Device: Galaxy Note
OS: Android 4.0.4
Comment 15 Cristian Nicolae (:xti) 2012-09-06 06:57:41 PDT
Wrong comment place :| ignore comment #14 please

Note You need to log in before you can comment on or make changes to this bug.