Closed
Bug 911195
Opened 11 years ago
Closed 11 years ago
Regression: Scroll address bar off screen broken
Categories
(Firefox OS Graveyard :: Gaia::Browser, defect)
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)
1.60 KB,
patch
|
daleharvey
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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?
Comment 2•11 years ago
|
||
Kats suggested on IRC that this is likely to be a platform or DOM change. It would be nice to get a regression window.
Keywords: regression,
regressionwindow-wanted
Updated•11 years ago
|
Whiteboard: [sprintready]
Comment 3•11 years ago
|
||
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/56363206
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.
Keywords: regressionwindow-wanted
Comment 5•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
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?
Comment 8•11 years ago
|
||
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.
Comment 9•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
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)
Comment 12•11 years ago
|
||
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)
Comment 13•11 years ago
|
||
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)
Comment 14•11 years ago
|
||
Dale said he could spare some time to look into this and bug 915137 which might be related.
Assignee: nobody → dale
Flags: needinfo?(dzbarsky)
Assignee | ||
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #803666 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•11 years ago
|
||
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8d223ec129b6
![]() |
||
Comment 19•11 years ago
|
||
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+
Assignee | ||
Comment 20•11 years ago
|
||
Fixed review comments
Attachment #803666 -
Attachment is obsolete: true
Attachment #804466 -
Flags: review+
Assignee | ||
Comment 21•11 years ago
|
||
Comment 22•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 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.
Description
•