Closed
Bug 1315233
Opened 8 years ago
Closed 8 years ago
Crash in mozilla::dom::PostMessageEvent::Run
Categories
(Core :: DOM: Events, defect, P1)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox50 | --- | unaffected |
firefox51 | + | fixed |
firefox52 | --- | fixed |
People
(Reporter: marcia, Assigned: kmag)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
baku
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
This bug was filed from the Socorro interface and is report bp-eacd3e4e-db24-4f20-845f-811762161104. ============================================================= Seen while looking at nightly crash stats: http://bit.ly/2fj02jL. Crash started with the 20161103030205 build.
Reporter | ||
Updated•8 years ago
|
status-firefox51:
--- → affected
Comment 2•8 years ago
|
||
This is a regression from bug 1308920
Blocks: 1308920
Flags: needinfo?(amarchesini) → needinfo?(kmaglione+bmo)
Comment 3•8 years ago
|
||
[Tracking Requested - why for this release]: bug 1308920 landed to FF50 too.
status-firefox50:
--- → ?
Assignee | ||
Comment 4•8 years ago
|
||
The crash is from a diagnostic assert, so 50 isn't affected. It looks like this is an add-on bug, but I'm still looking into the details. The most common correlation is with Norton Security Toolbar, and it definitely does some strange things with postMessage, but I only have access to obfuscated sources, and I can't tell yet whether this is being caused by the calls in PageMod scripts or in its other ad-hoc usage.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 5•8 years ago
|
||
Assuming I've managed to correctly decipher this over-engineered mess, I'm pretty sure the problem with the Norton add-ons is caused by the SDK's frame module: http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/ui/frame/model.js http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/ui/frame/view.js After a few dozen layers of indirection, the postMessage call from the add-on code to the frame object gets dispatched via a postMessage call to an <iframe> content window. Which normally works fine. Except in permanent private browsing mode, where the frame contents have a privateBrowsingId origin attribute, but the sandbox that eventually winds up calling postMessage does not.
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
This crash signature is the number 4 top crash for the 11-4 Nightly.
Keywords: regression,
topcrash
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8807918 [details] Bug 1315233: Allow window.postMessage from system principal with mismatched origin attributes. https://reviewboard.mozilla.org/r/90890/#review90732 Can you add a test?
Attachment #8807918 -
Flags: review?(amarchesini) → review+
Updated•8 years ago
|
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #9) > Can you add a test? Yeah. I was initially intending to, but none of the related OA changes to this code had any tests to go with them.
Keywords: leave-open
Comment 11•8 years ago
|
||
Pushed by maglione.k@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d74bafb4a41e Allow window.postMessage from system principal with mismatched origin attributes. r=baku
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d74bafb4a41e
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8808322 [details] Bug 1315233: Add test for postMessage from system principal to window with non-default originAttributes. https://reviewboard.mozilla.org/r/91150/#review91154
Attachment #8808322 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8807918 [details] Bug 1315233: Allow window.postMessage from system principal with mismatched origin attributes. Approval Request Comment [Feature/regressing bug #]: Bug 1308920 [User impact if declined]: This fixes a topcrash in Aurora, and also fixes some add-on compatibility issues with private browsing windows and permanent private browsing mode that trigger the crash. [Describe test coverage new/current, TreeHerder]: A follow-up patch adds tests for this issue. [Risks and why]: Low. This change simply exempts callers with the system principal from origin attribute checks when calling window.postMessage. Unprivileged callers are unaffected. [String/UUID change made/needed]: None.
Attachment #8807918 -
Flags: approval-mozilla-aurora?
Comment 16•8 years ago
|
||
Pushed by maglione.k@gmail.com: https://hg.mozilla.org/integration/autoland/rev/429f3d73d79b Add test for postMessage from system principal to window with non-default originAttributes. r=baku
Comment 17•8 years ago
|
||
Comment on attachment 8807918 [details] Bug 1315233: Allow window.postMessage from system principal with mismatched origin attributes. Fix a top crash. Take it in 51 aurora.
Attachment #8807918 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/429f3d73d79b
To be completely explicit - bug 1308920 never landed in 50? I don't see the mention of it, but I see the approval, and bug 1308920 comment 56 is pointing in that direction. If this is in 50, then we need to uplift all the way to beta.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #19) > To be completely explicit - bug 1308920 never landed in 50? I don't see the > mention of it, but I see the approval, and bug 1308920 comment 56 is > pointing in that direction. If this is in 50, then we need to uplift all > the way to beta. Bug 1308920 landed in 50, but the crash is caused by a diagnostic assert, which has no effect in non-debug beta builds.
Flags: needinfo?(kmaglione+bmo)
Comment 21•8 years ago
|
||
needs rebasing for aurora grafting 373563:d74bafb4a41e "Bug 1315233: Allow window.postMessage from system principal with mismatched origin attributes. r=baku" merging dom/base/nsGlobalWindow.cpp warning: conflicts while merging dom/base/nsGlobalWindow.cpp! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 22•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/51d34d1b38761455464623c72f0eae32da3d6bb1 https://hg.mozilla.org/releases/mozilla-aurora/rev/36f095d0dbfa344f551901b70437c88bee147e30
Flags: needinfo?(kmaglione+bmo)
Keywords: leave-open
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 23•8 years ago
|
||
Reopening, since a few crashes have now shown up in builds that have the fix. Those also seem to be extension issues, but of a different nature, and much lower volume.
Assignee | ||
Comment 24•8 years ago
|
||
It looks like the new crashes are caused by the Private Tab add-on: https://addons.mozilla.org/firefox/addon/private-tab/ It does things like opening channels and setting the usePrivateBrowsing flag on their load context to true, and then repeatedly doing so again at various intervals. Crashing is actually probably the correct behavior, in this case. Jorge or Andreas, can you please add a compatibility override to this add-on for Firefox 51+? We should also consider just outright blocklisting it.
Flags: needinfo?(jorge)
Flags: needinfo?(awagner)
Comment 25•8 years ago
|
||
I don't have access to either the compat override page or the blocklist tool.
Flags: needinfo?(awagner)
Updated•8 years ago
|
Flags: needinfo?(awilliamson)
Comment 26•8 years ago
|
||
Has/can someone notify the developer to explain what should be changed? The most recent update was only in August, mentioning 51, so it's not abandoned.
Assignee | ||
Comment 27•8 years ago
|
||
I'm not sure there's any fixing it, and if there is I can't explain how. The things its doing with protocol handlers are not safe, and aren't meant to be supported. The rest of the add-on is a terrifying mess of eval-based monkey patching that I really don't want to wade into. But the short of it is: The add-on is causing content windows with private browsing origin attributes to have access to other content windows with different private browsing origin attributes, but the same origin otherwise. Those windows are trying to communicate with each other, and causing a crash. That's what needs to be fixed: content windows with different origin attributes should never be able to get references to each other.
Comment 28•8 years ago
|
||
I added the compat override and directed the developer to #c27
Flags: needinfo?(awilliamson)
Assignee | ||
Updated•8 years ago
|
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 29•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #27) > The things its doing with protocol handlers are not safe, and aren't meant to be > supported. So, custom nsIProtocolHandler implementation, that will change nsILoadContext.usePrivateBrowsing "aren't supported"? Or how implement such feature in safe way? > The rest of the add-on is a terrifying mess of eval-based monkey > patching that I really don't want to wade into. Really? The main part is just about .usePrivateBrowsing = true right after "TabOpen" event. Also there is only wrappers like var orig = fn; fn = function() { // do something return orig.aply(this, arguments); }; > But the short of it is: The add-on is causing content windows with private > browsing origin attributes to have access to other content windows with > different private browsing origin attributes, but the same origin otherwise. > Those windows are trying to communicate with each other, and causing a > crash. That's what needs to be fixed: content windows with different origin > attributes should never be able to get references to each other. OK. How I can modify "the same origin" things or do something else from Private Tab side to forbid such access? Any API?
Comment 30•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #24) > It does things like opening channels and setting the usePrivateBrowsing flag > on their load context to true, and then repeatedly doing so again at various > intervals. Crashing is actually probably the correct behavior, in this case. Where is "repeatedly doing so again at various intervals"? https://github.com/Infocatcher/Private_Tab/blob/0.2.0/protocol.jsm#L94-L98 Or this is about something else? Please explain. Also... The most important question. How correctly implement private tabs?
Updated•8 years ago
|
Flags: needinfo?(jorge)
Comment 31•8 years ago
|
||
This bug report is about the crashes, which were resolved. Please move the technical discussion to the forum or one of our other developer channels: https://developer.mozilla.org/en-US/Add-ons#Contact_us
Comment 32•8 years ago
|
||
(In reply to Jorge Villalobos [:jorgev] from comment #31) > This bug report is about the crashes, which were resolved. Please move the > technical discussion to the forum or one of our other developer channels: > https://developer.mozilla.org/en-US/Add-ons#Contact_us OK, I've created separated bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1318388
Assignee | ||
Comment 33•8 years ago
|
||
(In reply to Infocatcher from comment #29) > (In reply to Kris Maglione [:kmag] from comment #27) > > The things its doing with protocol handlers are not safe, and aren't meant to be > > supported. > > So, custom nsIProtocolHandler implementation, that will change > nsILoadContext.usePrivateBrowsing "aren't supported"? Correct. That information should come from the load context of the docShell, which should match the origin attributes of the principals loaded into the docShell, and should never change after the docShell is created. You're currently (repeatedly) changing the private browsing status of a docShell when it loads a certain channel, which is very much not supported. > > The rest of the add-on is a terrifying mess of eval-based monkey > > patching that I really don't want to wade into. > > Really? Yes, but let's not get into it. If it were up to me, any amount of eval-based monkey patching would have been banned a long time ago. And when it's tied up with intricate, security-sensitive code, it makes things especially difficult to reason about. But, in any case, that comment was meant for Andrew, who should know the context that I intended it to be taken in. > OK. How I can modify "the same origin" things or do something else from > Private Tab side to forbid such access? Any API? The only plausibly safe way to do this is to set the private browsing ID on the origin attributes of the tab's top-level docShell when it's created, but "plausibly safe" is as far as I think that will take you. (In reply to Infocatcher from comment #30) > (In reply to Kris Maglione [:kmag] from comment #24) > > It does things like opening channels and setting the usePrivateBrowsing flag > > on their load context to true, and then repeatedly doing so again at various > > intervals. Crashing is actually probably the correct behavior, in this case. > > Where is "repeatedly doing so again at various intervals"? > https://github.com/Infocatcher/Private_Tab/blob/0.2.0/protocol.jsm#L94-L98 Yes. > Also... The most important question. > How correctly implement private tabs? See above, but I'm not sure there's really any "correct" way without a lot of platform changes, at this point. But we can continue this conversation in the other bug.
Comment 34•8 years ago
|
||
As an user of the PrivateTab addon I hope Firefox developers will actively help to make this addon compatible. It should be a native feature but at least you should make thinks easy to people developing it.
You need to log in
before you can comment on or make changes to this bug.
Description
•