Closed Bug 911195 Opened 6 years ago Closed 6 years ago

Regression: Scroll address bar off screen broken

Categories

(Firefox OS Graveyard :: Gaia::Browser, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.2 FC (16sep)

People

(Reporter: benfrancis, Assigned: daleharvey)

References

Details

(Keywords: regression, Whiteboard: [sprintready] burirun1)

Attachments

(1 file, 1 obsolete file)

STR:
* Navigate to a long web page in the browser
* Scroll down
Expected:
* Address bar animates off screen
Actual:
* Address bar stays in place and see an error in the console.

This is on master with mozilla-central nightly build on Unagi

The error is 

Error: Permission denied to access property 'top'" {file: "app://browser.gaiamobile.org/js/browser.js" line: 501}]

This is when the browser tries to read the top value from the mozbrowserscroll event.
reproduced by going to bbc.co.uk and navigating around there and to a few articles. I didn't see any errors, but the awesomebar never hid. Is this sprintready?
Blocks: 835152
Kats suggested on IRC that this is likely to be a platform or DOM change. It would be nice to get a regression window.
Whiteboard: [sprintready]
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/56363206
QA Contact: gbennett
Below is the regression window.

0824 - Issue not present
Environmental Variables
Device: Buri 1.2.0
Build ID: 20130824040225
Gecko: http://hg.mozilla.org/mozilla-central/rev/17143a9a0d83
Gaia: 42d88f26420b6c58621d1b68675d512fd10c9fcb
Platform Version: 26.0a1

0826 - Issue Exists
Environmental Variables
Device: Buri 1.2.0
Build ID: 20130826040201
Gecko: http://hg.mozilla.org/mozilla-central/rev/4887845b1142
Gaia: 62fbe42afb482ed78de6591edb3eddd30d919f00
Platform Version: 26.0a1

I couldn't check 0825 as every time I tried flashing it would hang on the animated fox startup screen.
Thanks! The gecko pushlog for that range is: hg.mozilla.org/mozilla-central/pushloghtml?fromchange=17143a9a0d83&tochange=4887845b1142

In that range I see bug 908692 might be related. Tom, any ideas on why window.top might be broken in b2g content processes?
Flags: needinfo?(evilpies)
I actually pushed some followup today where we stop using .top in Bug 910431. Maybe that fixes it?
Flags: needinfo?(evilpies)
Oh wait, I'm sorry. I thought it was trying to read window.top but it's actually trying to read the .top property from the mozbrowserasyncscroll event. At https://git.mozilla.org/?p=releases/gaia.git;a=blob;f=apps/browser/js/browser.js;h=503b17acdadc01d7b16981de5d85e4ae28668e9a;hb=HEAD#l507

Still, Tom, can you look at the pushlog range in comment 5 to see if you know why this is happening and what can be done to fix it?
Bug 908692 couldn't have caused it because that code is part of browser/ which is not used in b2g.

From looking at the regression range, I'd bet this was caused by bug 906901. Either that, or it's a regression from the Gaia tree. But bug 906901 is directly relevant.
Ah, I think you are right. I didn't look for the right things in the regression range diff. Thanks!

:dzbarsky, thoughts on making the "top" property of the async scroll event accessible again?
Blocks: 906901
Flags: needinfo?(dzbarsky)
Hmm, I'm not sure what's going on here.  Looks like the top property exists, but the browser app can't use it because its failing security checks.  I can try to debug this later today.
Flags: needinfo?(dzbarsky)
Any update?
Flags: needinfo?(dzbarsky)
Bobby, any idea why the access is being denied here?  Perhaps wrapping with a null global at http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp#298 is incorrect?
Flags: needinfo?(bobbyholley+bmo)
It seems pretty broken to me that the Dictionary bindings have a method called "ToObject" that converts to a JSVal. But I guess that's a separate issue. I filed bug 914775 for that. Either way, it looks like the codegen for AsyncScrollEventDetail::ToObject doesn't use |parentObject| at all, so that's not the issue here.

Can you figure out exactly where the security error is being thrown? That's the next step here. An easy trick is to do Math.sin(2) right before the problematic call. Then, break on js::math_sin. Then, once you hit the breakpoint, break on JSContext::setPendingException(), which is likely to tell you where the exception is being reported. It may be an explicit call to JS_ReportError, or it may be binding code that's converting an XPCOM failure code from further down.
Flags: needinfo?(bobbyholley+bmo)
Dale said he could spare some time to look into this and bug 915137 which might be related.
Assignee: nobody → dale
Flags: needinfo?(dzbarsky)
So this bug was cause by https://bugzilla.mozilla.org/show_bug.cgi?id=911195

The event gets passed from BEP.cpp to BEP.jsm which gets sent straight to browser.js, browser.js is getting an XRay wrapped object so doesnt have access to the properties. bholley mentioned on irc that :

> because AutoSafeJSContext forcibly puts us in the SafeJSContextGlobal's compartment
> which is this dummy thing that isn't what you want
> you need to explicitly enter a compartment after you declare your AutoSafeJSContext

Which needs to be an compartment that browser.js can access.

I can write a patch but need a little guidance in how to set the correct compartment
Flags: needinfo?(dzbarsky)
You probably need to AutoEnterCompartment with the global for the popup frame element.  Bobby helped me with this when I first wrote that patch, so he probably understands it better than me.
Flags: needinfo?(dzbarsky)
Comment on attachment 803666 [details] [diff] [review]
Properly compartment scroll event object

>+  nsCOMPtr<nsIGlobalObject> globalObject = doc->GetParentObject();

GetScopeObject, please.  GetParentObject is an implementation detail.

>+  JSObject* globalJSObject = globalObject->GetGlobalJSObject();

This needs to happen after the AutoSafeJSContext.  Like so:

  AutoSafeJSContext cx;
  JS::Rooted<JSObject*> globalJSObject(cx, globalObject->GetGlobalJSObject());
  NS_ENSURE_TRUE(globalJSObject, NS_ERROR_UNEXPECTED);

r=me with that fixed.
Attachment #803666 - Flags: review?(bzbarsky) → review+
Fixed review comments
Attachment #803666 - Attachment is obsolete: true
Attachment #804466 - Flags: review+
Whiteboard: [sprintready] → [sprintready] burirun1
https://hg.mozilla.org/mozilla-central/rev/9a8916b16e6e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
You need to log in before you can comment on or make changes to this bug.